Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storybook-builder-vite-parent@0.0.13 - Adding vue2 support and updating yarn workspaces #66

Conversation

matthewhardern
Copy link

This PR adds the ability to use vue2 as well as vue 3. I have modified the server and the vite config. I have also added prettier/standalone and prettier/parser-html to your optimizeDeps.js until storybook fix addon-docs. I have also modified the yarn workspaces to get it working. This is not my favourite thing in the world, but with yarn 2 I am unsure how to specify a package individually and instead have resorted to the "installConfig": {"hoistingLimits": "workspaces"} as thats what I could find from the docs, It would be good if we could just specify the package like yarn 1, so if anyone knows how to do that then let me know and I will make the change. If not I will come back with future PR to make it work like that after reading more of the docs. Also had to remove docgen on vue2 as had no idea how to get it working as have not used the package before, but its still on vue3.

@matthewhardern matthewhardern mentioned this pull request Jul 9, 2021
@eirslett
Copy link
Collaborator

eirslett commented Jul 9, 2021

Thanks a lot for the work!

@IanVS can you please help me review this PR? I don't know yarn or Vue 2 too well.

…on files to 0.0.13 of this package to make use of the new working version
@matthewhardern
Copy link
Author

Thanks a lot for the work!

@IanVS can you please help me review this PR? I don't know yarn or Vue 2 too well.

No problem, I have just also removed my local yalc as forgot to remove it when pushing originally.

@IanVS
Copy link
Member

IanVS commented Jul 12, 2021

@matthewhardern it doesn't seem that yalc was actually removed.

with yarn 2 I am unsure how to specify a package individually

Can you explain a bit more about the problem you were having?

I think it's a good idea to use nmHoistingLimits like you've done here, but can you explain why you chose dependencies instead of workspaces? I wonder if using workspaces would more more similar to how hoisting is done in real consuming projects, where npm and yarn will both flatten out the tree as much as they can. Also, you're already overriding the setting to workspaces inside the new vue2 example you've made, so maybe it can be set to workspaces at the root, and you could remove that override from the vue2 example.

@jbo023
Copy link

jbo023 commented Jul 12, 2021

Thanks, for the merge request, that was good timing, just a day before i was searching for a solution.

I got this code working for our existing vue2+vuetify+storybook project where we are in the process of moving to vite.
Seems to just work, but if we find any problems we will report them here.

@matthewhardern
Copy link
Author

@matthewhardern it doesn't seem that yalc was actually removed.

with yarn 2 I am unsure how to specify a package individually

Can you explain a bit more about the problem you were having?

I think it's a good idea to use nmHoistingLimits like you've done here, but can you explain why you chose dependencies instead of workspaces? I wonder if using workspaces would more similar to how hoisting is done in real consuming projects, where npm and yarn will both flatten out the tree as much as they can. Also, you're already overriding the setting to workspaces inside the new vue2 example you've made, so maybe it can be set to workspaces at the root, and you could remove that override from the vue2 example.

@IanVS sorry not sure what you mean I have removed it from package json and added it to git ignore file, can't see the .yalc folder has been committed, but I may be wrong?

On the problem, in yarn 1 you used to be able to do be able to specify a package to not be hoisted. It's not really a problem if people are happy with "workspaces" at the global level then I am happy with that, the reason I put them at the example level is it was only relevant to vue 2 and 3 conflicts. I will set the workspaces at the root level.

.gitignore Outdated Show resolved Hide resolved
@matthewhardern
Copy link
Author

I have removed the git ignore yalc text and have added workspaces at the global level.

Copy link
Member

@IanVS IanVS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems pretty straightforward and reasonable, and I'm happy with the yarn change. However, I think that the change to the version number should be done separately, not in this PR.

packages/storybook-builder-vite/package.json Outdated Show resolved Hide resolved
@matthewhardern
Copy link
Author

This seems pretty straightforward and reasonable, and I'm happy with the yarn change. However, I think that the change to the version number should be done separately, not in this PR.

Completely agree have undone the version number change

@marsanla
Copy link

Are you going to merge this PR?

@IanVS
Copy link
Member

IanVS commented Jul 19, 2021

@matthewhardern it looks like you reverted the version number change in the examples, but not in the main package.json or storybook-builder-vite.

@matthewhardern
Copy link
Author

@matthewhardern it looks like you reverted the version number change in the examples, but not in the main package.json or storybook-builder-vite.

My apologies, I have just updated

@IanVS
Copy link
Member

IanVS commented Jul 19, 2021

It looks like there's an example failing with Error: Cannot find module 'vite'

@matthewhardern
Copy link
Author

It looks like there's an example failing with Error: Cannot find module 'vite'

The reason for this is because vite is no longer being hoisted to the top-level node modules, and as its a peer dependency of the storybook-builder-vite package it no longer has access to vite when building. I will have a quick think about how best to deal with it.

@matthewhardern
Copy link
Author

@IanVS I have had to change the approach here as doing it with workspaces hoisting set to workspaces just was not going to work without adding a significant number of dependencies to the storybook-builder-vite package. The approach I went with which may not be what people want was to alias vue3 dependency using "vue3": "npm:vue@3.1.4", then in the builder and server add a check to see if vue 3 is being aliased, if it is resolve the root of the aliased package and then use this path to get the vue.esm-bundler.js. Let me know if you have issues with this or any better ideas?

@Toilal
Copy link

Toilal commented Aug 9, 2021

Any advice to test this branch on a project ? It seems it's not possible to install it from git : lerna/lerna#2074

@jbo023
Copy link

jbo023 commented Aug 10, 2021

@Toilal you can just use npm pack in the storybook-builder-vite folder and use the tgz file directly in your package.json

"storybook-builder-vite": "./storybook-builder-vite-0.0.13.tgz",

@IanVS
Copy link
Member

IanVS commented Aug 18, 2021

Sorry I haven't had a chance to review. It would be great if someone using vue2 could confirm that this indeed does work for them.

@nadirabbas
Copy link

@IanVS I just tried it with Vue 2, for some reason, the builder still looks for vue/dist/vue.esm-bundler.js (which I guess is available in Vue 3?).

image

@rpauls
Copy link

rpauls commented Aug 20, 2021

Just tested the changes in a new project.
Storybook itself starts up without errors in the console, but only displays the loading page within the browser.

$ start-storybook -p 6006
info @storybook/vue v6.3.7
info 
info => Loading presets
Could not find aliased vue3 continuing with vue 3 as "vue"
info => Using prebuilt manager
Pre-bundling dependencies:
  airbnb-js-shims
  lodash/mapValues
  lodash/pick
  lodash/isFunction
  lodash/isString
  (...and 66 more)
(this will be run only when your dependencies or config have changed)
╭─────────────────────────────────────────────────────╮
│                                                     │
│   Storybook 6.3.7 started                           │
│   1.48 s for preview                                │
│                                                     │
│    Local:            http://localhost:6006/         │
│    On your network:  http://192.168.178.43:6006/    │
│                                                     │
╰─────────────────────────────────────────────────────╯
13:03:54 [vite] new dependencies found: @storybook/vue, updating...
13:03:54 [vite] ✨ dependencies updated, reloading page...

Screenshot 2021-08-20 at 13-04-04 Storybook

After a manual refresh in the browser Storybook works as expected including HMR.

Edit:
Browser Console output directly after automatic opening of Storybook instance:

Screenshot 2021-08-20 at 13 08 50

@Vnthf Vnthf mentioned this pull request Oct 14, 2021
@eirslett
Copy link
Collaborator

eirslett commented Jan 7, 2022

Closing, see #115 (comment)

@eirslett eirslett closed this Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants