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

Ember: Fix @storybook/ember #23435

Merged
merged 24 commits into from Dec 11, 2023
Merged

Ember: Fix @storybook/ember #23435

merged 24 commits into from Dec 11, 2023

Conversation

francois2metz
Copy link
Contributor

@francois2metz francois2metz commented Jul 13, 2023

Closes #20653

What I did

This PR fixes the ember addon on the latest 7.0 release. It was also broken on the 6.0.0 release since #17843.
The code has been updated to install ember with the webpack builder and the patch #17843 has been reverted.
To make thinks easier to test, I added two sandbox templates for ember 3 and ember 4.

How to test

Well, that's the fun part

Ember 3

  1. yarn task --task dev --template ember/3-js --start-from=install
  2. Go to sandbox/ember-3-js
  3. Edit the package.json to update ember-auto-import to ^2.0.0.
  4. Edit the package.json to update ember-qunit to ^6.0.0.
  5. Run yarn
  6. Run yarn add @storybook/ember-cli-storybook
  7. Run yarn build && yarn storybook
  8. Access the stories and see that the preview is working \o/

Ember 4

  1. yarn task --task dev --template ember/default-js --start-from=install
  2. Access the stories and see that the preview is working \o/

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

🦋 Canary release

This pull request has been released as version 0.0.0-pr-23435-sha-bd98198e. Install it by pinning all your Storybook dependencies to that version.

More information
Published version 0.0.0-pr-23435-sha-bd98198e
Triggered by @shilman
Repository 1024pix/storybook
Branch fix-ember-3.28
Commit bd98198e
Datetime Sun Aug 20 15:21:51 UTC 2023 (1692544911)
Workflow run 5918186521

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=23435

@francois2metz
Copy link
Contributor Author

With storybookjs/ember-cli-storybook#183, I updated the sandbox template to ember 4.12 and it worked.

@shilman shilman self-assigned this Jul 17, 2023
@esbanarango
Copy link

esbanarango commented Jul 20, 2023

@francois2metz Thank you for putting the effort here 🙏 . Looking forward to this PR to be merged 👍

@sebasjimenez10
Copy link

Small bump to get this merged soon! 🙏 Thanks for contributing @francois2metz !

@francois2metz
Copy link
Contributor Author

Hi @sebasjimenez10, @esbanarango and thanks! Could you test to see if it's working for you?

@yannbertrand
Copy link

yannbertrand commented Jul 24, 2023

I did try locally from a fresh storybook clone with Node18 and Ember4.

I followed the steps described in the description. A few notes:

  • I checkout this PR with GitHub's gh.
  • When launching the first command yarn task --task dev --template ember/default-js --start-from=install, Storybook tries to launch itself in dev mode after the sandbox step (no error until dev). I just ignored the error and ctrl+c.
ERR! Make sure this directory exists, or omit the -s (--static-dir) option.
ERR!     at parseStaticDir (/Users/yannbertrand/Developer/storybook/sandbox/ember-default-js/node_modules/@storybook/core-server/dist/presets/common-preset.js:6021:11)
  • Then yarn add @storybook/ember-cli-storybook gives an error because I don't have the local npm registry. I ignore it as well and comment the npmRegistryServer config as explained above.
➤ YN0027: @storybook/ember-cli-storybook@unknown can't be resolved to a satisfying range
➤ YN0001: RequestError: connect ECONNREFUSED ::1:6001
    at ClientRequest.<anonymous> (/Users/yannbertrand/Developer/storybook/sandbox/ember-default-js/.yarn/releases/yarn-3.6.1.cjs:195:14340)
  • I copy/paste the change in node_modules/@storybook/ember-cli-storybook/lib/util.js.
  • yarn build works fine.
  • Then yarn storybook gives these errors in the console.
ModuleNotFoundError: Module not found: Error: Can't resolve './stories/components' in '/Users/yannbertrand/Developer/storybook/sandbox/ember-default-js'
 using description file: /Users/yannbertrand/Developer/storybook/sandbox/ember-default-js/package.json (relative path: .)
    Field 'browser' doesn't contain a valid alias configuration
    using description file: /Users/yannbertrand/Developer/storybook/sandbox/ember-default-js/package.json (relative path: ./stories/components)
  • Storybook launches on port 6006 as you can see on the following screenshots, but there is some errors. There is probably some deprecated stuff on Storybook's template config.

image
image
image

@yannbertrand
Copy link

I retried from a fresh clone, using Node16 and got the same results.

@ndelangen
Copy link
Member

The code changes relating to the core & sandboxes all look 👍 to me. I can't really say anything meaningful about the ember changes.

@yannbertrand
Copy link

yannbertrand commented Jul 26, 2023

@shilman
Copy link
Member

shilman commented Dec 10, 2023

@francois2metz I got CI green in #25162. If you can update this PR with those changes, we should be able to get this one green

@francois2metz
Copy link
Contributor Author

I fixed the template names and it worked. Nothing that I can understand.

@gossi
Copy link
Contributor

gossi commented Dec 11, 2023

It's greeeeeeeeeeeeeeeeeeeen 🥳 🎉

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Great job @francois2metz !! Thanks so much for bearing with us on this one 😅

@shilman shilman changed the title Ember: Fix @storybook/ember Ember: Fix @storybook/ember and add v4 support Dec 11, 2023
@shilman shilman changed the title Ember: Fix @storybook/ember and add v4 support Ember: Fix @storybook/ember Dec 11, 2023
@shilman shilman merged commit 7dedaf4 into storybookjs:next Dec 11, 2023
49 of 50 checks passed
@github-actions github-actions bot mentioned this pull request Dec 11, 2023
9 tasks
@francois2metz
Copy link
Contributor Author

thanks @shilman. I would love to backport this PR on the 7.x branch. What do you think?

@chasegiunta
Copy link

@shilman is it not possible to get this backported to 7.x? I'm assuming a stable 8 release is still quite a while away.

@gossi
Copy link
Contributor

gossi commented Jan 12, 2024

Yeah, a backport to v7 sounds nice and we can have a stable sb@v7 with an stable ember version.

But also:

  • This only affects ember people
  • We can use this with an alpha of sb, as long as this works.
  • We can even upgrade the alpha version of sb, as long as this works.
  • If it stops working, we can lock the alpha version of sb@8 that was the last one working and try every now and then to upgrade
  • sb@7 never worked for ember, so we can make skip this (like php6 😂)

I think it makes more sense to improve the DX for working with sb@8 at ember and aim at for a good experience once that hits a final release (given there is low capa from ember side, this might be converging on either schedules).

That is a very sober view on the practical side of things, which I see very realistic - I'm still with you on how that looks and rest assured my perfectionistic artistic sense is hammering this message into me, too 😬

@shilman
Copy link
Member

shilman commented Jan 12, 2024

@chasegiunta we typically only patch very small, non-disruptive changes and I would say this doesn't qualify. Fortunately, we're shooting for an 8.0 release at end of Feb, so it isn't too far out at this point.

@gossi gossi mentioned this pull request Feb 3, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Fresh install Storybook with Ember framework produces a ModuleNotFoundError