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: Fix process is not defined #18367

Merged
merged 3 commits into from Jun 12, 2022

Conversation

joshwooding
Copy link
Contributor

@joshwooding joshwooding commented May 31, 2022

Issue: #18366

What I did

Moved memoizerific to dependencies.

memoizerific has package.json#browser

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented May 31, 2022

☁️ Nx Cloud Report

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

@joshwooding
Copy link
Contributor Author

Not sure if global should be moved to dependencies in the packages too?

@shilman shilman requested a review from ndelangen May 31, 2022 11:09
@shilman
Copy link
Member

shilman commented May 31, 2022

@joshwooding we are prebundling dev dependencies in @storybook/router, so this might not do what you think it does. hopefully @ndelangen can clarify here.

@joshwooding
Copy link
Contributor Author

@joshwooding we are prebundling dev dependencies in @storybook/router, so this might not do what you think it does. hopefully @ndelangen can clarify here.

Yeah, moving the dependencies to "dependencies" prevents prebundling from what I could tell. Memoizerific isn't suitable for prebundling since it has a browser implementation of it's code and qs was previously moved elsewhere but not sure if those changes are needed elsewhere... looking forward to the clarification :)

@shilman
Copy link
Member

shilman commented Jun 6, 2022

This looks like it was addressed a different way in #18412

@Andarist if you see this can you please take a look at @joshwooding 's comment above? Many thanks! 🙏

@Andarist
Copy link
Contributor

Andarist commented Jun 6, 2022

Yep, some dependencies are just not suitable for prebundling - qs is definitely one of them and if memoizerific has browser field then it isn't either. In general, I think that prebundling should only happen to specific libraries - when it solves a problem for you, but by default, it shouldn't be applied and things should be left in dependencies. My motivation is to avoid exactly such problems like this one and to avoid duplicating code as what gets prebundled can't be deduplicated by a package manager. Of course, YMMV

This looks like it was addressed a different way in #18412

I think I'm applying a similar fix there (moving a package from devDep to dep, but I've only moved qs there), just for different reasons. I was solely investigating a PnP-incompatibility reported by the user and this PR here focuses on fixing packages that were prebundled using a wrong env target (prebundled for node when the final env of a consumer is a browser). Both problems can usually be fixed in the same way.

@joshwooding
Copy link
Contributor Author

I've updated this PR to only tackle memoizerific since qs was fixed in #18412

Copy link
Contributor

@Andarist Andarist left a comment

Choose a reason for hiding this comment

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

LGTM - however, I don't think that I should be the one to merge this since I'm mostly an external contributor here 😉

@shilman shilman changed the title Fix process is not defined Deps: Fix process is not defined Jun 12, 2022
@shilman shilman merged commit d969777 into storybookjs:next Jun 12, 2022
@joshwooding joshwooding deleted the fix-process-is-not-defined branch June 12, 2022 22:23
@shilman shilman added patch:yes Bugfix & documentation PR that need to be picked to main branch patch:done Patch/release PRs already cherry-picked to main/release branch labels Jun 12, 2022
@shilman shilman changed the title Deps: Fix process is not defined Core: Fix process is not defined Jun 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core dependencies patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants