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

issue #6763 : set a better value for process in the manager webpack config #6767

Merged
merged 1 commit into from
May 17, 2019

Conversation

libetl
Copy link
Member

@libetl libetl commented May 11, 2019

Issue:

What I did

I have fixed the trouble with libraries making use of the process variable.
In the manager configuration, the variable is set to true,
Which causes several troubles when using additional libraries (like crypto)
So instead of true, I am setting a more relevant value
this also fixes issue #6763

How to test

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

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

@vercel
Copy link

vercel bot commented May 11, 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-bugfix-process-variable-set-to-true.storybook.now.sh

@libetl libetl requested review from shilman and ndelangen May 11, 2019 17:50
new CaseSensitivePathsPlugin(),
new Dotenv({ silent: true }),
// graphql sources check process variable
new DefinePlugin({
process: JSON.stringify(true),
process: { browser: true, env: stringified },
NODE_ENV: JSON.stringify(process.env.NODE_ENV),
Copy link
Member Author

@libetl libetl May 11, 2019

Choose a reason for hiding this comment

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

is the above NODE_ENV key necessary or is it a bug by the way (global.NODE_ENV) ?
leaving as it is for now.

Copy link
Member

Choose a reason for hiding this comment

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

It's intentional, from #6215

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Thanks @libetl!! 🙏

@shilman
Copy link
Member

shilman commented May 12, 2019

@ndelangen can you take a look at this? i think you wrote the code it's replacing

@libetl
Copy link
Member Author

libetl commented May 17, 2019

@shilman is this still necessary ?

@shilman
Copy link
Member

shilman commented May 17, 2019

@libetl As far as I know this is still necessary. @ndelangen ping

@ndelangen ndelangen merged commit 3b6e1d0 into next May 17, 2019
@ndelangen ndelangen deleted the bugfix/process-variable-set-to-true branch May 17, 2019 11:06
@libetl
Copy link
Member Author

libetl commented May 17, 2019

Yet another conflict... With myself.

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.

3 participants