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

fix(@angular/cli): disable node global #8130

Merged
merged 2 commits into from
Oct 23, 2017

Conversation

filipesilva
Copy link
Contributor

The node only global object had been left in because it used to cause a build size regression with Angular.

This doesn't seem to be the case anymore so it should be removed because it causes problematic interactions with some libraries.

Fix #5804
Supersedes #7931

@filipesilva
Copy link
Contributor Author

Note: [email protected] and up do not support old IE versions: https://github.com/webpack/webpack-dev-server#caveats

IgorMinar added a commit to IgorMinar/angular that referenced this pull request Oct 21, 2017
This will be fixed in CLI once angular/angular-cli#8130 lands.

-rw-r--r--  1 iminar  eng   14942 Oct 20 22:23 dist/0.b19e913fbdd6507d346b.chunk.js
-rw-r--r--  1 iminar  eng    1535 Oct 20 22:23 dist/inline.5d66b81ec9e01af9d28d.bundle.js
-rw-r--r--  1 iminar  eng  528395 Oct 20 22:23 dist/main.e36bb99245ca52ae546f.bundle.js
-rw-r--r--  1 iminar  eng   37205 Oct 20 22:23 dist/polyfills.0dfca732c5a075c110d0.bundle.js
IgorMinar added a commit to IgorMinar/angular that referenced this pull request Oct 21, 2017
This will be fixed in CLI once angular/angular-cli#8130 lands.

-rw-r--r--  1 iminar  eng   14942 Oct 20 22:23 dist/0.b19e913fbdd6507d346b.chunk.js
-rw-r--r--  1 iminar  eng    1535 Oct 20 22:23 dist/inline.5d66b81ec9e01af9d28d.bundle.js
-rw-r--r--  1 iminar  eng  528395 Oct 20 22:23 dist/main.e36bb99245ca52ae546f.bundle.js
-rw-r--r--  1 iminar  eng   37205 Oct 20 22:23 dist/polyfills.0dfca732c5a075c110d0.bundle.js
IgorMinar added a commit to IgorMinar/angular that referenced this pull request Oct 21, 2017
This will be fixed in CLI once angular/angular-cli#8130 lands.

-rw-r--r--  1 iminar  eng   14942 Oct 20 22:23 dist/0.b19e913fbdd6507d346b.chunk.js
-rw-r--r--  1 iminar  eng    1535 Oct 20 22:23 dist/inline.5d66b81ec9e01af9d28d.bundle.js
-rw-r--r--  1 iminar  eng  528395 Oct 20 22:23 dist/main.e36bb99245ca52ae546f.bundle.js
-rw-r--r--  1 iminar  eng   37205 Oct 20 22:23 dist/polyfills.0dfca732c5a075c110d0.bundle.js
@Wykks
Copy link
Contributor

Wykks commented Oct 21, 2017

Whoops I found an issue with these changes :
Uncaught ReferenceError: process is not defined
https://github.com/mapbox/mapbox-sdk-js/blob/f1012d33d5315866a66bf97d27d0dc0f66807161/vendor/invariant.js#L23
Only with ng serve, prod serve works fine.

@filipesilva
Copy link
Contributor Author

Ok I see. It's quite common for libs to try and check the env if available so we shouldn't disable it.

Thanks a bunch for testing this, it would have gotten through otherwise.

The node only `global` object had been left in because it used to cause a build size regression with Angular.

This doesn't seem to be the case anymore so it should be removed because it causes problematic interactions with some libraries.

Fix angular#5804
Supersedes angular#7931
@filipesilva
Copy link
Contributor Author

@Wykks done, the PR doesn't remove process anymore.

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

I confirmed that this helps quite a bit for aio. I'm not sure what all the implications are for other projects.

IgorMinar added a commit to IgorMinar/angular that referenced this pull request Oct 23, 2017
This will be fixed in CLI once angular/angular-cli#8130 lands.

-rw-r--r--  1 iminar  eng   14942 Oct 20 22:23 dist/0.b19e913fbdd6507d346b.chunk.js
-rw-r--r--  1 iminar  eng    1535 Oct 20 22:23 dist/inline.5d66b81ec9e01af9d28d.bundle.js
-rw-r--r--  1 iminar  eng  528395 Oct 20 22:23 dist/main.e36bb99245ca52ae546f.bundle.js
-rw-r--r--  1 iminar  eng   37205 Oct 20 22:23 dist/polyfills.0dfca732c5a075c110d0.bundle.js
@hansl hansl merged commit a167a33 into angular:master Oct 23, 2017
@filipesilva filipesilva deleted the disable-global branch October 23, 2017 22:10
@IgorMinar
Copy link
Contributor

IgorMinar commented Oct 24, 2017 via email

@filipesilva
Copy link
Contributor Author

@IgorMinar I think the error on the message you linked was due to patching the CLI files to disable global but not having updated webpack-dev-server with it (as this PR does). The webpack-dev-server version was needed to fix webpack/webpack-dev-server#1147 (fixed by webpack/webpack-dev-server#1148).

petebacondarwin pushed a commit to IgorMinar/angular that referenced this pull request Oct 24, 2017
This will be fixed in CLI once angular/angular-cli#8130 lands.

-rw-r--r--  1 iminar  eng   14942 Oct 20 22:23 dist/0.b19e913fbdd6507d346b.chunk.js
-rw-r--r--  1 iminar  eng    1535 Oct 20 22:23 dist/inline.5d66b81ec9e01af9d28d.bundle.js
-rw-r--r--  1 iminar  eng  528395 Oct 20 22:23 dist/main.e36bb99245ca52ae546f.bundle.js
-rw-r--r--  1 iminar  eng   37205 Oct 20 22:23 dist/polyfills.0dfca732c5a075c110d0.bundle.js
@hccampos
Copy link

Seems like pdfjs is broken due to this change. It (or one of its dependencies) tries to load the browserify Buffer, which checks the environment.

@vecernik
Copy link

can you make global configurable in angular-cli.json ? It also breaks Faye.

@IgorMinar
Copy link
Contributor

IgorMinar commented Oct 27, 2017 via email

@vecernik
Copy link

@IgorMinar I agree. I have found no way how to get Faye (and possibly others) to work without global.

petebacondarwin pushed a commit to IgorMinar/angular that referenced this pull request Oct 29, 2017
This will be fixed in CLI once angular/angular-cli#8130 lands.

-rw-r--r--  1 iminar  eng   14942 Oct 20 22:23 dist/0.b19e913fbdd6507d346b.chunk.js
-rw-r--r--  1 iminar  eng    1535 Oct 20 22:23 dist/inline.5d66b81ec9e01af9d28d.bundle.js
-rw-r--r--  1 iminar  eng  528395 Oct 20 22:23 dist/main.e36bb99245ca52ae546f.bundle.js
-rw-r--r--  1 iminar  eng   37205 Oct 20 22:23 dist/polyfills.0dfca732c5a075c110d0.bundle.js
IgorMinar added a commit to IgorMinar/angular that referenced this pull request Nov 1, 2017
This will be fixed in CLI once angular/angular-cli#8130 lands.

-rw-r--r--  1 iminar  eng   14942 Oct 20 22:23 dist/0.b19e913fbdd6507d346b.chunk.js
-rw-r--r--  1 iminar  eng    1535 Oct 20 22:23 dist/inline.5d66b81ec9e01af9d28d.bundle.js
-rw-r--r--  1 iminar  eng  528395 Oct 20 22:23 dist/main.e36bb99245ca52ae546f.bundle.js
-rw-r--r--  1 iminar  eng   37205 Oct 20 22:23 dist/polyfills.0dfca732c5a075c110d0.bundle.js
gkalpak pushed a commit to IgorMinar/angular that referenced this pull request Nov 1, 2017
This will be fixed in CLI once angular/angular-cli#8130 lands.

-rw-r--r--  1 iminar  eng   14942 Oct 20 22:23 dist/0.b19e913fbdd6507d346b.chunk.js
-rw-r--r--  1 iminar  eng    1535 Oct 20 22:23 dist/inline.5d66b81ec9e01af9d28d.bundle.js
-rw-r--r--  1 iminar  eng  528395 Oct 20 22:23 dist/main.e36bb99245ca52ae546f.bundle.js
-rw-r--r--  1 iminar  eng   37205 Oct 20 22:23 dist/polyfills.0dfca732c5a075c110d0.bundle.js
matsko pushed a commit to angular/angular that referenced this pull request Nov 1, 2017
This will be fixed in CLI once angular/angular-cli#8130 lands.

-rw-r--r--  1 iminar  eng   14942 Oct 20 22:23 dist/0.b19e913fbdd6507d346b.chunk.js
-rw-r--r--  1 iminar  eng    1535 Oct 20 22:23 dist/inline.5d66b81ec9e01af9d28d.bundle.js
-rw-r--r--  1 iminar  eng  528395 Oct 20 22:23 dist/main.e36bb99245ca52ae546f.bundle.js
-rw-r--r--  1 iminar  eng   37205 Oct 20 22:23 dist/polyfills.0dfca732c5a075c110d0.bundle.js

PR Close #19702
matsko pushed a commit to angular/angular that referenced this pull request Nov 1, 2017
This will be fixed in CLI once angular/angular-cli#8130 lands.

-rw-r--r--  1 iminar  eng   14942 Oct 20 22:23 dist/0.b19e913fbdd6507d346b.chunk.js
-rw-r--r--  1 iminar  eng    1535 Oct 20 22:23 dist/inline.5d66b81ec9e01af9d28d.bundle.js
-rw-r--r--  1 iminar  eng  528395 Oct 20 22:23 dist/main.e36bb99245ca52ae546f.bundle.js
-rw-r--r--  1 iminar  eng   37205 Oct 20 22:23 dist/polyfills.0dfca732c5a075c110d0.bundle.js

PR Close #19702
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mapboxgl is incompatible with the CLI in a production build
7 participants