Open mrauhu opened 2 years ago
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
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:
sourceMap
option in the tsconfig.json
https://www.typescriptlang.org/tsconfig#sourceMap
--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:
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.
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!
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.
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?
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?
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
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(`
Weird, this all worked for me previously. Maybe this is an issue with an updated dependency. I'm going to do a little investigation.
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- thank you for suggestion, I will check out the Playwright solution.
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 codeESLint and Prettier https://github.com/eirslett/storybook-builder-vite/pull/193
package.json
to.prettierrc
..editorconfig
with the Prettier standard.Testing
Documentation https://github.com/eirslett/storybook-builder-vite/pull/200
CONTRIBUTING.md
.Docker https://github.com/eirslett/storybook-builder-vite/pull/202
docker-compose.yml
with CI and Storybook services.Minor changes to the package code: syntax and typing
Run
lint
https://github.com/eirslett/storybook-builder-vite/pull/196lint
on all codebase.packages/storybook-builder-vite
directory..git-blame-ignore-revs
file.TypeScript https://github.com/eirslett/storybook-builder-vite/pull/195
tsconfig.json
for comfort debugging).*.js
to*.ts
.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.