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(core): use shakeable global definitions #29929

Closed
wants to merge 2 commits into from

Conversation

filipesilva
Copy link
Contributor

@filipesilva filipesilva commented Apr 16, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

The ngDevMode and ngI18nClosureMode are special in that they should be set to false on production builds in order to shake out code associated with it.

Angular CLI does this in https://github.com/angular/angular-cli/blob/5fc1f2499cbe57f9a95e4b0dfced130eb3a8046d/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/common.ts#L279-L282.

But in #28689 the toplevel usage was changed from ngDevMode to global['ngDevMode'] (and the same for ngI18nClosureMode). This indirection prevents the static analysis in Terser from effecting the replacement.

What is the new behavior?

The variables that should be defined globally are synced between Angular core and CLI.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@mary-poppins
Copy link

You can preview bb7b732 at https://pr29929-bb7b732.ngbuilds.io/.

The `ngDevMode` and `ngI18nClosureMode` are special in that they should be set to `false` on production builds in order to shake out code associated with it.

Angular CLI does this in https://github.com/angular/angular-cli/blob/5fc1f2499cbe57f9a95e4b0dfced130eb3a8046d/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/common.ts#L279-L282.

But in angular#28689 the toplevel usage was changed from `ngDevMode` to `global['ngDevMode']` (and the same for `ngI18nClosureMode`). This indirection prevents the static analysis in Terser from effecting the replacement.
@mary-poppins
Copy link

You can preview b44260f at https://pr29929-b44260f.ngbuilds.io/.

@alexeagle alexeagle added the target: major This PR is targeted for the next major release label Apr 16, 2019
@filipesilva filipesilva added the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 17, 2019
@filipesilva filipesilva requested review from a team as code owners April 17, 2019 10:43
@mary-poppins
Copy link

You can preview c194b32 at https://pr29929-c194b32.ngbuilds.io/.

@alexeagle alexeagle added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 17, 2019
@filipesilva filipesilva requested review from vikerman and removed request for mhevery and ocombe April 17, 2019 14:46
@benlesh benlesh added the area: core Issues related to the framework runtime label Apr 17, 2019
@ngbot ngbot bot added this to the needsTriage milestone Apr 17, 2019
@benlesh
Copy link
Contributor

benlesh commented Apr 17, 2019

presubmit

@benlesh benlesh closed this in e5905bb Apr 18, 2019
benlesh pushed a commit that referenced this pull request Apr 18, 2019
@filipesilva filipesilva deleted the fix-globaldefs branch April 18, 2019 08:44
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Apr 24, 2019
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Apr 24, 2019
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Apr 24, 2019
alexeagle pushed a commit to angular/angular-cli that referenced this pull request Apr 24, 2019
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
The `ngDevMode` and `ngI18nClosureMode` are special in that they should be set to `false` on production builds in order to shake out code associated with it.

Angular CLI does this in https://github.com/angular/angular-cli/blob/5fc1f2499cbe57f9a95e4b0dfced130eb3a8046d/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/common.ts#L279-L282.

But in angular#28689 the toplevel usage was changed from `ngDevMode` to `global['ngDevMode']` (and the same for `ngI18nClosureMode`). This indirection prevents the static analysis in Terser from effecting the replacement.

PR Close angular#29929
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
@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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants