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

Core: Followup changing CJS entrypoints to ESM #17927

Merged
merged 2 commits into from Apr 11, 2022
Merged

Core: Followup changing CJS entrypoints to ESM #17927

merged 2 commits into from Apr 11, 2022

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Apr 11, 2022

Following up on #17868 -- rather than importing the ESM files, we also need to actually write ESM code! Thanks @IanVS

I would also be OK with just reverting the original PR and removing all these files in 7.0 (three of the 5 files don't work already, as noted below, anyway).

Now that #17875 has gone out, there is less obvious need for this change.

@nx-cloud
Copy link

nx-cloud bot commented Apr 11, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 454b023. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

Comment on lines +1 to +2
// eslint-disable-next-line import/no-unresolved
export * from './dist/esm/html';
Copy link
Member Author

Choose a reason for hiding this comment

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

@ndelangen these three "entrypoints" (@storybook-components/html, @storybook-router/utils and @storybook-theming/create) don't actually work now we pre-bundle. I included the change just to call it out in the PR, but right now these files don't point to anything (thus the linting issue)

Copy link
Member

Choose a reason for hiding this comment

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

hmm, that seems like a pretty big deal.. I do not remember why we had those separate entrypoints to begin with, do you remember why we introduced those?\

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, I know nothing about them. @shilman?

Copy link
Member

@shilman shilman Apr 11, 2022

Choose a reason for hiding this comment

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

@tmeasday @ndelangen

@storybook/components/html was a grouping for MDX docs elements. we deprecated it a year ago, so if we have an unintentional breaking change there it's not the end of the world:

https://github.com/storybookjs/storybook/blob/next/lib/components/src/html.tsx

I'm less sure about the other two. @ndelangen I think that's more your territory.

@tmeasday tmeasday added core maintenance User-facing maintenance tasks labels Apr 11, 2022
@tmeasday tmeasday requested a review from shilman April 11, 2022 01:52
@yannbf yannbf changed the title Followup changing CJS entrypoints to ESM Core: Followup changing CJS entrypoints to ESM Apr 11, 2022
@yannbf yannbf merged commit 5c026fe into next Apr 11, 2022
@yannbf yannbf deleted the 17868-followup branch April 11, 2022 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants