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

Don't redefine process variable #6991

Merged
merged 1 commit into from
Jun 6, 2019
Merged

Don't redefine process variable #6991

merged 1 commit into from
Jun 6, 2019

Conversation

Hypnosphi
Copy link
Member

@Hypnosphi Hypnosphi commented Jun 6, 2019

Issue: by default, webpack injects process/browser import whenever process global variable is used. Our DefinePlugin overrides it which breaks packages that use stuff like process.cwd(). One example is unified package used by react-markdown internally.

Honestly, I don't remember why I had to add this line which were later copypasted to other places. Anyway, GraphQL addon seems to work OK after my current changes.

@Hypnosphi Hypnosphi added bug configuration babel / webpack patch:yes Bugfix & documentation PR that need to be picked to main branch core labels Jun 6, 2019
@vercel
Copy link

vercel bot commented Jun 6, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-dont-redefine-process.storybook.now.sh

@tmeasday
Copy link
Member

tmeasday commented Jun 6, 2019

There is another (recently merged?) PR that was fixing this: #6946?

This solution seems better to me however

@Hypnosphi
Copy link
Member Author

There is another (recently merged?) PR that was fixing this: #6946?

It only adds browser field, while there's a lot more in https://github.com/defunctzombie/node-process/blob/master/browser.js

@tmeasday
Copy link
Member

tmeasday commented Jun 6, 2019

This solution seems better to me however

🤷‍♂

@shilman shilman added this to the 5.1.x milestone Jun 6, 2019
@shilman
Copy link
Member

shilman commented Jun 6, 2019

I was just blindly copy-pasting a fix from another PR. As long as this doesn't break things, I agree this is much cleaner. Any good way to test this?

@Hypnosphi
Copy link
Member Author

@shilman
Copy link
Member

shilman commented Jun 6, 2019

Ok, I'm not sure exactly what that proves, but I'll take your word for it 😘

That proves that browser is set to true?

@Hypnosphi
Copy link
Member Author

@shilman No, this proves that defining the whole process variable on our side isn't really needed. That becomes the responsibility of default webpack behavior.

@Hypnosphi
Copy link
Member Author

Hypnosphi commented Jun 6, 2019

The compiled code for process usage looks roughly like this, you can check it yourself in develomment, in vendors file:

/* WEBPACK VAR INJECTION */(function(process) {
  <...>
/* WEBPACK VAR INJECTION */}.call(this, __webpack_require__("../../node_modules/process/browser.js")))

And process/browser.js of course defines the browser field among others: https://github.com/defunctzombie/node-process/blob/master/browser.js#L156

@cloud-walker
Copy link

I use react-markdown and I've upgraded storybook to v5.1 and part of my stories are like this right now:

Schermata 2019-06-06 alle 15 00 49

@Hypnosphi
Copy link
Member Author

@cloud-walker yeah, that's exactly the issue this PR fixes

@shilman shilman merged commit 1385e62 into next Jun 6, 2019
@shilman shilman deleted the dont-redefine-process branch June 6, 2019 18:01
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jun 6, 2019
shilman added a commit that referenced this pull request Jun 6, 2019
Don't redefine `process` variable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug configuration babel / webpack core 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.

4 participants