builder-vite icon indicating copy to clipboard operation
builder-vite copied to clipboard

Suggestions for improving maintainability

Open mrauhu opened this issue 3 years ago • 9 comments

Hello.

I want to implement best practices in tooling that inspired by Storybook and Vite repositories.

The goal is to make it easier for maintainers to participate in the project.

Related: #11

Checklist

Zero changes to the storybook-builder-vite package code

ESLint and Prettier https://github.com/eirslett/storybook-builder-vite/pull/193

  • [x] Move the Prettier configuration from package.json to .prettierrc.
  • [x] Synchronize .editorconfig with the Prettier standard.
  • [x] Add ESLint with Prettier config.
  • [x] Add the TypeScript plugin to ESlint.
  • [x] Run Prettier before ESLint

Testing

  • [x] Add Jest for unit testing.
  • [ ] Add Cypress for E2E testing.
  • [x] Running tests on a pull request and publish events.

Documentation https://github.com/eirslett/storybook-builder-vite/pull/200

  • [x] Create a guide for developers in CONTRIBUTING.md.
  • [x] Add scripts for running examples.

Docker https://github.com/eirslett/storybook-builder-vite/pull/202

  • [x] Create docker-compose.yml with CI and Storybook services.
  • [x] Add scripts for Docker compose services.

Minor changes to the package code: syntax and typing

Run lint https://github.com/eirslett/storybook-builder-vite/pull/196

  • [x] Run lint on all codebase.
  • [x] Fix code inspection warnings from a JetBrains IDE for the packages/storybook-builder-vite directory.
  • [x] Create .git-blame-ignore-revs file.

TypeScript https://github.com/eirslett/storybook-builder-vite/pull/195

  • [x] Use TypeScript (with right tsconfig.json for comfort debugging).
  • [x] Convert all codebase from *.js to *.ts.
  • [x] Typization.

Minor changes in examples package.json and Github Actions

  • [ ] Workspaces: use NPM or Pnpm, because Yarn is not working out of the box like it should be.

    Details: https://github.com/eirslett/storybook-builder-vite/issues/188#issuecomment-1005851871

@eirslett, @IanVS

May I create a one pull request with all suggested features?

Best wishes, Sergey.

mrauhu avatar Dec 26 '21 05:12 mrauhu

Thanks for the suggestions, @mrauhu, and for checking before submitting a PR. I think it's good to talk about this kind of thing before putting in the work.

I can't speak for @eirslett, and this is his project, but I'm personally on board with most of your suggestions. One area I disagree in though is the use of TypeScript. Normally I am a big fan of TypeScript, and I tend to use it in my projects. However, the experimental nature of this builder means that I'm often mucking around with it inside my node_modules of the application I'm building, and many times I've appreciated that the source is not compiled, and I can work directly with the code rather than a minified or compiled output. For that reason, at least for now, I'd recommend sticking with plain old javascript without a build step.

As for switching to a different package manager, yarn is used by Storybook itself, so if it's falling down on the job we're probably doing something wrong. Can you explain what you mean by not working as it should?

For eslint & prettier, there are different ways to get them to work together. One is to run prettier through eslint, and the other is to disable all the eslint style rules, and then use prettier to do formatting and eslint for other linting. Which approach do you propose?

Perhaps it's best to make some small PRs instead of one large one with all the changes. I think we could get some big wins from e2e cypress tests and/or chromatic snapshots on the examples. But so far @eirslett has not had a chance to sign up for Chromatic, and none of the rest of us have permissions to do so (https://github.com/eirslett/storybook-builder-vite/issues/154).

Again, thanks for the suggestions and for the offer to help.

IanVS avatar Dec 28 '21 00:12 IanVS

@IanVS

One area I disagree in though is the use of TypeScript. ... I can work directly with the code rather than a minified or compiled output.

If we talking about simplification of debugging process, we can:

  • enable the sourceMap option in the tsconfig.json https://www.typescriptlang.org/tsconfig#sourceMap
    • and distribute source maps with the package;
  • use the --enable-source-maps argument for Node.js https://nodejs.org/docs/latest-v16.x/api/cli.html#--enable-source-maps.

Right now, the Storybook project is using the TypeScript for builders, as example the Webpack 5 builder:

  • https://github.com/storybookjs/storybook/blob/6796c210ac84bdd70355e8a308ea15d01aed582a/lib/builder-webpack5/tsconfig.json
    • https://github.com/storybookjs/storybook/blob/6796c210ac84bdd70355e8a308ea15d01aed582a/tsconfig.json

So, I suggest use the same way.

For eslint & prettier, there are different ways to get them to work together. One is to run prettier through eslint, and the other is to disable all the eslint style rules, and then use prettier to do formatting and eslint for other linting. Which approach do you propose?

I prefer use ESLint with Prettier plugin:

.eslintrc.js

module.exports = {
  // ...
  extends: [
    'plugin:@typescript-eslint/recommended',
    'plugin:prettier/recommended',
  ],
};

As for switching to a different package manager, yarn is used by Storybook itself, so if it's falling down on the job we're probably doing something wrong. Can you explain what you mean by not working as it should?

I tried to run the Vue example multiple times with different conditions (with and without: node_modules, yarn.lock, etc), but Yarn is not working not in my system, nor in Docker:

  • Failed:

    yarn
    yarn workspace example-vue storybook
    
  • Also failed:

    yarn
    cd packages/example-vue
    yarn storybook
    
  • Not working at all:

    cd packages/example-vue 
    yarn
    yarn storybook
    
  • This is the way

    I made a small change in package.json files:

    {
      // ...
      "devDependencies": {
         // ...
         "storybook-builder-vite": "file:../storybook-builder-vite"
       }
    }
    

    And then ran the example with two commands:

    npm i
    npm -w packages/example-vue storybook
    

Perhaps it's best to make some small PRs instead of one large one with all the changes.

Okay, I made small PRs.

mrauhu avatar Jan 03 '22 16:01 mrauhu

This all sounds good to me!

I share @IanVS's concerns about working with TypeScript in Node.js: the debugging experience is not ideal. But it looks like you have some good solutions for that!

Maybe we need a "Contributing" section in the README (or a separate CONTRIBUTING.md) for how to set up the development environment, debug with Node/TypeScript etc.

Looking forward to the PRs!

eirslett avatar Jan 03 '22 23:01 eirslett

I think my concerns about typescript are mostly that it's more annoying to work with compiled output in node_modules, rather than the raw source. But maybe it's fine as long as we don't compile down to es5, and we configure typescript to just strip out the types.

IanVS avatar Jan 04 '22 02:01 IanVS

I think we need more information here about the issues with yarn. You've only said that it's not working. Are you receiving errors with storybook, are you receiving errors with yarn?

joshwooding avatar Jan 05 '22 14:01 joshwooding

I think we need more information here about the issues with yarn.

@joshwooding basically, I can't run examples using Yarn.

You've only said that it's not working. Are you receiving errors with storybook, are you receiving errors with yarn?

Reproduction of the Yarn workspace related issue

Let's try running the Vue example in a clean environment:

git clone https://github.com/eirslett/storybook-builder-vite
cd storybook-builder-vite
yarn
yarn workspace example-vue storybook

System

Output
C:\Work\storybook\storybook-builder-vite>yarn workspace example-vue storybook
info @storybook/vue3 v6.4.3
info
info => Loading presets
Pre-bundling dependencies:
  @base2/pretty-print-object
  @emotion/core
  @emotion/is-prop-valid
  @emotion/styled
  @mdx-js/react
  (...and 77 more)
(this will be run only when your dependencies or config have changed)
info => Using prebuilt manager
╭────────────────────────────────────────────────────╮
│                                                    │
│   Storybook 6.4.3 for Vue3 started                 │
│   14 s for preview                                 │
│                                                    │
│    Local:            http://localhost:51916/       │
│    On your network:  http://172.20.192.1:51916/    │
│                                                    │
│   A new version (6.4.9) is available!              │
│                                                    │
│   Upgrade now: npx sb@latest upgrade               │
│                                                    │
│   Read full changelog: https://git.io/fhFYe        │
│                                                    │
╰────────────────────────────────────────────────────╯
18:20:13 [vite] Internal server error: Failed to resolve import "@vue/server-renderer" from "stories\Button.vue". Does the file exist?
  Plugin: vite:import-analysis
  File: C:/Work/storybook/storybook-builder-vite/packages/example-vue/stories/Button.vue
  65 |  import { ssrRenderAttrs as _ssrRenderAttrs, ssrInterpolate as _ssrInterpolate } from "@vue/server-renderer"
  66 |
  67 |  function _sfc_ssrRender(_ctx, _push, _parent, _attrs, $props, $setup, $data, $options) {
     |                                         ^
  68 |    _push(`<button${
  69 |      _ssrRenderAttrs(_mergeProps({
      at formatError (C:\Work\storybook\storybook-builder-vite\node_modules\vite\dist\node\chunks\dep-7e125991.js:42587:46)
      at TransformContext.error (C:\Work\storybook\storybook-builder-vite\node_modules\vite\dist\node\chunks\dep-7e125991.js:42583:19)
      at normalizeUrl (C:\Work\storybook\storybook-builder-vite\node_modules\vite\dist\node\chunks\dep-7e125991.js:80909:26)
      at async TransformContext.transform (C:\Work\storybook\storybook-builder-vite\node_modules\vite\dist\node\chunks\dep-7e125991.js:81049:57)
      at async Object.transform (C:\Work\storybook\storybook-builder-vite\node_modules\vite\dist\node\chunks\dep-7e125991.js:42803:30)
      at async doTransform (C:\Work\storybook\storybook-builder-vite\node_modules\vite\dist\node\chunks\dep-7e125991.js:57478:29)

Errors

Server-side

[vite] Internal server error: Failed to resolve import "@vue/server-renderer" from "stories\*.vue". Does the file exist?

image

Client-side

GET http://localhost:51916/stories/*.vue net::ERR_ABORTED 404 (Not Found) *.stories.js

image

Maybe it's an OS related issue?!

Let's go, try the same in a isolated environment.

Docker

Dockerfile

FROM node:lts-alpine

ARG PORT=7007
ENV PORT=${PORT}

RUN apk add --no-cache git

WORKDIR /app

RUN git clone https://github.com/eirslett/storybook-builder-vite

WORKDIR /app/storybook-builder-vite

RUN yarn
RUN 

EXPOSE ${PORT}/tcp

CMD yarn workspace example-vue storybook -p ${PORT}

docker-compose.yml

version: '3'

services:
  example:
    build: .
    ports:
      - "127.0.0.1:7007:7007"
docker-compose up --build example

Errors

Not changed at all:

Server-side

[vite] Internal server error: Failed to resolve import "@vue/server-renderer" from "stories/*.vue". Does the file exist?

image

Client-side

GET http://localhost:7007/stories/*.vue net::ERR_ABORTED 404 (Not Found) *.stories.js

image

So, after this experiments, I suggest to change the package manager.

mrauhu avatar Jan 05 '22 15:01 mrauhu

I think we need more information here about the issues with yarn.

@joshwooding basically, I can't run examples using Yarn.

You've only said that it's not working. Are you receiving errors with storybook, are you receiving errors with yarn?

Reproduction of the Yarn workspace related issue

Let's try running the Vue example in a clean environment:


git clone https://github.com/eirslett/storybook-builder-vite

cd storybook-builder-vite

yarn

yarn workspace example-vue storybook

System

<summary>Output</summary>



C:\Work\storybook\storybook-builder-vite>yarn workspace example-vue storybook

info @storybook/vue3 v6.4.3

info

info => Loading presets

Pre-bundling dependencies:

  @base2/pretty-print-object

  @emotion/core

  @emotion/is-prop-valid

  @emotion/styled

  @mdx-js/react

  (...and 77 more)

(this will be run only when your dependencies or config have changed)

info => Using prebuilt manager

╭────────────────────────────────────────────────────╮

│                                                    │

│   Storybook 6.4.3 for Vue3 started                 │

│   14 s for preview                                 │

│                                                    │

│    Local:            http://localhost:51916/       │

│    On your network:  http://172.20.192.1:51916/    │

│                                                    │

│   A new version (6.4.9) is available!              │

│                                                    │

│   Upgrade now: npx sb@latest upgrade               │

│                                                    │

│   Read full changelog: https://git.io/fhFYe        │

│                                                    │

╰────────────────────────────────────────────────────╯

18:20:13 [vite] Internal server error: Failed to resolve import "@vue/server-renderer" from "stories\Button.vue". Does the file exist?

  Plugin: vite:import-analysis

  File: C:/Work/storybook/storybook-builder-vite/packages/example-vue/stories/Button.vue

  65 |  import { ssrRenderAttrs as _ssrRenderAttrs, ssrInterpolate as _ssrInterpolate } from "@vue/server-renderer"

  66 |

  67 |  function _sfc_ssrRender(_ctx, _push, _parent, _attrs, $props, $setup, $data, $options) {

     |                                         ^

  68 |    _push(`<button${

  69 |      _ssrRenderAttrs(_mergeProps({

      at formatError (C:\Work\storybook\storybook-builder-vite\node_modules\vite\dist\node\chunks\dep-7e125991.js:42587:46)

      at TransformContext.error (C:\Work\storybook\storybook-builder-vite\node_modules\vite\dist\node\chunks\dep-7e125991.js:42583:19)

      at normalizeUrl (C:\Work\storybook\storybook-builder-vite\node_modules\vite\dist\node\chunks\dep-7e125991.js:80909:26)

      at async TransformContext.transform (C:\Work\storybook\storybook-builder-vite\node_modules\vite\dist\node\chunks\dep-7e125991.js:81049:57)

      at async Object.transform (C:\Work\storybook\storybook-builder-vite\node_modules\vite\dist\node\chunks\dep-7e125991.js:42803:30)

      at async doTransform (C:\Work\storybook\storybook-builder-vite\node_modules\vite\dist\node\chunks\dep-7e125991.js:57478:29)

Errors

Server-side


[vite] Internal server error: Failed to resolve import "@vue/server-renderer" from "stories\*.vue". Does the file exist?

image

Client-side


GET http://localhost:51916/stories/*.vue net::ERR_ABORTED 404 (Not Found) *.stories.js

image

Maybe it's an OS related issue?!

Let's go, try the same in a isolated environment.

Docker

Dockerfile


FROM node:lts-alpine



ARG PORT=7007

ENV PORT=${PORT}



RUN apk add --no-cache git



WORKDIR /app



RUN git clone https://github.com/eirslett/storybook-builder-vite



WORKDIR /app/storybook-builder-vite



RUN yarn

RUN 



EXPOSE ${PORT}/tcp



CMD yarn workspace example-vue storybook -p ${PORT}

docker-compose.yml


version: '3'



services:

  example:

    build: .

    ports:

      - "127.0.0.1:7007:7007"


docker-compose up --build example

Errors

Not changed at all:

Server-side


[vite] Internal server error: Failed to resolve import "@vue/server-renderer" from "stories/*.vue". Does the file exist?

image

Client-side


GET http://localhost:7007/stories/*.vue net::ERR_ABORTED 404 (Not Found) *.stories.js

image

So, after this experiments, I suggest to change the package manager.

Weird, this all worked for me previously. Maybe this is an issue with an updated dependency. I'm going to do a little investigation.

joshwooding avatar Jan 06 '22 01:01 joshwooding

Found this while looking for issues around using emotion with this builder, worth mentioning that we recently moved from cypress to playwright. It greatly simplified our tests, and removed a tonne of code needed to do basic tasks in cypress. Highly recommend checking it out, cypress is incredibly painful to develop on compared to playwright in my experience.

Codex- avatar Feb 14 '22 01:02 Codex-

@Codex- thank you for suggestion, I will check out the Playwright solution.

mrauhu avatar Feb 14 '22 10:02 mrauhu