-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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: Fix auto-title in webpack5 #16913
Conversation
Nx Cloud ReportCI ran the following commands for commit 8ddd297. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch
Sent with 💌 from NxCloud. |
Nx Cloud ReportWe didn't find any information for the current pull request with the commit 8ddd297. Check the Nx Cloud Github Integration documentation for more information. Sent with 💌 from NxCloud. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
Core: Fix auto-title in webpack5
I think this fix should go to manager webpack config as well. I had to patch like this in my project:
It seems manager config is missing the shim right now. |
@bebraw is the patch something specific to a particular addon or your project's config? i don't think storybook core needs it, but i'm happy to add it if it's causing enough people problems |
Without the additional fix, it was crashing at the path import within auto title logic for me.
It would be consistent to have the same shimming behavior for manager compilation as well. I imagine there's something in my project that's triggering the title logic.
… On 6. Dec 2021, at 14.40, Michael Shilman ***@***.***> wrote:
@bebraw is the patch something specific to a particular addon? i don't think storybook core needs it, but i'm happy to add it if it's causing enough people problems
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
Hmmm, I wonder why auto title would be running on the manager side @bebraw. Any chance of a stack trace? |
I will try to reduce it. It's definitely weird it's getting picked up in my particular case.
… On 6. Dec 2021, at 21.39, Tom Coleman ***@***.***> wrote:
Hmmm, I wonder why auto title would be running on the manager side @bebraw. Any chance of a stack trace?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
@tmeasday Here's the trace, https://gist.github.com/bebraw/210b6a2411f53c5063eca40d88bdeace . To keep it simple, I disabled all the addons. |
@bebraw from that trace it looks like you are importing That's a little unexpected but I suppose if you are doing it, others will be too, in which case this polyfill is necessary, certainly outside of 7.0. |
That's a good catch. I see this kind of usage In my case it seems it's even dead code so I can safely remove it if |
@bebraw -- I wonder why you are using |
@tmeasday It's some older code I didn't write. It's possible the initial usage was simply wrong. |
Ok given that, I'm going to close that PR |
Issue: #16891 #16903
What I did
Webpack5 no longer includes
path
polyfills by default, so we need to add it by hand.self-merging @tmeasday
How to test
I did
sb link
the repro from #16891 and it failed before the change & worked after