Suggestions for improving maintainability #188

mrauhu · 2021-12-26T05:10:56Z

IanVS · 2021-12-28T00:22:52Z

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 (#154).

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

mrauhu · 2022-01-03T16:53:34Z

@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.

eirslett · 2022-01-03T23:14:12Z

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!

IanVS · 2022-01-04T02:25:03Z

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.

joshwooding · 2022-01-05T14:52:55Z

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?

mrauhu · 2022-01-05T15:58:24Z

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?

Client-side

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

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?

Client-side

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

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

joshwooding · 2022-01-06T01:45:37Z

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?

Client-side


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

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?

Client-side


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

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.

Codex- · 2022-02-14T01:04:56Z

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.

mrauhu · 2022-02-14T10:27:15Z

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

mrauhu mentioned this issue Dec 26, 2021

More maintainers needed #11

Closed

mrauhu mentioned this issue Jan 5, 2022

Improving maintainability: ESLint and Prettier #193

Merged

5 tasks

mrauhu mentioned this issue Jan 5, 2022

Improving maintainability: TypeScript #195

Merged

3 tasks

mrauhu mentioned this issue Jan 6, 2022

Improving maintainability: run lint #196

Merged

3 tasks

joshwooding mentioned this issue Jan 7, 2022

Bump @vitejs/plugin-vue to 1.10.2 #199

Merged

This was referenced Jan 8, 2022

Improving maintainability: Documentation #200

Merged

Improving maintainability: Docker #202

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggestions for improving maintainability #188

Suggestions for improving maintainability #188

mrauhu commented Dec 26, 2021 •

edited by IanVS

Loading

IanVS commented Dec 28, 2021

mrauhu commented Jan 3, 2022 •

edited

Loading

eirslett commented Jan 3, 2022

IanVS commented Jan 4, 2022

joshwooding commented Jan 5, 2022

mrauhu commented Jan 5, 2022 •

edited

Loading

joshwooding commented Jan 6, 2022

Reproduction of the Yarn workspace related issue

System

Errors

Server-side

Client-side

Docker

Errors

Server-side

Client-side

Codex- commented Feb 14, 2022

mrauhu commented Feb 14, 2022

Suggestions for improving maintainability #188

Suggestions for improving maintainability #188

Comments

mrauhu commented Dec 26, 2021 • edited by IanVS Loading

Checklist

Zero changes to the storybook-builder-vite package code

ESLint and Prettier #193

Testing

Documentation #200

Docker #202

Minor changes to the package code: syntax and typing

Run lint #196

TypeScript #195

Minor changes in examples package.json and Github Actions

IanVS commented Dec 28, 2021

mrauhu commented Jan 3, 2022 • edited Loading

eirslett commented Jan 3, 2022

IanVS commented Jan 4, 2022

joshwooding commented Jan 5, 2022

mrauhu commented Jan 5, 2022 • edited Loading

Reproduction of the Yarn workspace related issue

System

Errors

Server-side

Client-side

Docker

Errors

Server-side

Client-side

joshwooding commented Jan 6, 2022

Reproduction of the Yarn workspace related issue

System

Errors

Server-side

Client-side

Docker

Errors

Server-side

Client-side

Codex- commented Feb 14, 2022

mrauhu commented Feb 14, 2022

mrauhu commented Dec 26, 2021 •

edited by IanVS

Loading

Zero changes to the `storybook-builder-vite` package code

Run `lint` #196

Minor changes in examples `package.json` and Github Actions

mrauhu commented Jan 3, 2022 •

edited

Loading

mrauhu commented Jan 5, 2022 •

edited

Loading