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

Run prebundle script without browser: true in Rollup config #17947

Merged
merged 1 commit into from
Apr 14, 2022

Conversation

Andarist
Copy link
Contributor

This is a tricky PR - but ultimately I think it's for the better. The problem is that there is a chance that you might want to use browser: true for some packages but not for some other ones (like Emotion). There is no way to control this per-package though and since you don't control the consuming environment, you can't assume that it's going to be a browser - so, in a way, this setting was used incorrectly here.

However, there is some chance that not using it might break something - it's quite hard to test it though unless you already know what it might break or if automated tests (or manual smoke tests) show the problem.

@Andarist Andarist requested a review from ndelangen April 13, 2022 08:53
@nx-cloud
Copy link

nx-cloud bot commented Apr 13, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 7b370a7. 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.

@ndelangen ndelangen self-assigned this Apr 14, 2022
Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

ok, I get it.. looks like all lights on green based on the CI, and this might actually fix https://github.com/kylegach/storybook-theming-bundling-bug-2

@ndelangen ndelangen merged commit 9fd1827 into next Apr 14, 2022
@ndelangen ndelangen deleted the prebundle-without-browser branch April 14, 2022 08:14
@arcanis
Copy link
Contributor

arcanis commented Apr 19, 2022

I suspect this PR might be related to #17978

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.

None yet

3 participants