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

refactor(upgrade): rename module constant to avoid webpack bug #30058

Closed

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Apr 23, 2019

When targeting ES2015 (as is the default in cli@8), const is not downleveled to var and thus declaring const module throws an error due to webpack wrapping the code in a function call with a module argument (even when compiling for the web environment).

Related: webpack/webpack#7369

Fixes #30050.

@gkalpak gkalpak requested a review from a team as a code owner April 23, 2019 15:06
@gkalpak gkalpak added area: upgrade Issues related to AngularJS → Angular upgrade APIs effort1: hours action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release risk: low refactoring Issue that involves refactoring or code-cleanup and removed cla: yes labels Apr 23, 2019
@ngbot ngbot bot added this to the Backlog milestone Apr 23, 2019
@googlebot
Copy link

☹️ Sorry, but only Googlers may change the label cla: yes.

@mary-poppins
Copy link

You can preview f73d431 at https://pr30058-f73d431.ngbuilds.io/.

When targeting ES2015 (as is the default in cli@8), `const` is not
downleveled to `var` and thus declaring `const module` throws an error
due to webpack wrapping the code in a function call with a `module`
argument (even when compiling for the `web` environment).

Related: webpack/webpack#7369

Fixes angular#30050
@gkalpak gkalpak force-pushed the refactor-upgrade-rename-module branch from f73d431 to d97a8b7 Compare April 23, 2019 15:18
@mary-poppins
Copy link

You can preview d97a8b7 at https://pr30058-d97a8b7.ngbuilds.io/.

@benlesh benlesh added the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 23, 2019
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.

omg. can we get this fixed in webpack?

@gkalpak
Copy link
Member Author

gkalpak commented Apr 24, 2019

Hopefully 😅

@alfaproject
Copy link
Contributor

But wouldn't this be a problem in a node environment if the webpack bug is fixed for the web environment? I'm thinking Angular Universal

@gkalpak gkalpak removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 24, 2019
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Apr 24, 2019

Presubmit, rerun due to flakes.

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Apr 24, 2019
@IgorMinar IgorMinar modified the milestones: Backlog, version 8 Apr 24, 2019
@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Apr 24, 2019
@IgorMinar IgorMinar added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Apr 24, 2019
@IgorMinar
Copy link
Contributor

merge-assistance: g3 failure looks like a flake

@AndrewKushnir AndrewKushnir removed the target: patch This PR is targeted for the next patch release label Apr 25, 2019
@AndrewKushnir AndrewKushnir added the target: major This PR is targeted for the next major release label Apr 25, 2019
@AndrewKushnir
Copy link
Contributor

@gkalpak merging to patch branch failed due to conflicts, so I merged this PR to master only. Could you please create a separate PR and target patch branch? Thank you.

@gkalpak gkalpak deleted the refactor-upgrade-rename-module branch April 25, 2019 08:48
gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 25, 2019
…ular#30058)

When targeting ES2015 (as is the default in cli@8), `const` is not
downleveled to `var` and thus declaring `const module` throws an error
due to webpack wrapping the code in a function call with a `module`
argument (even when compiling for the `web` environment).

Related: webpack/webpack#7369

Fixes angular#30050

PR Close angular#30058
gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 25, 2019
…ular#30058)

When targeting ES2015 (as is the default in cli@8), `const` is not
downleveled to `var` and thus declaring `const module` throws an error
due to webpack wrapping the code in a function call with a `module`
argument (even when compiling for the `web` environment).

Related: webpack/webpack#7369

Fixes angular#30050

PR Close angular#30058
gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 25, 2019
…ular#30058)

When targeting ES2015 (as is the default in cli@8), `const` is not
downleveled to `var` and thus declaring `const module` throws an error
due to webpack wrapping the code in a function call with a `module`
argument (even when compiling for the `web` environment).

Related: webpack/webpack#7369

Fixes angular#30050

PR Close angular#30058
gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 25, 2019
With angular#30058, the ngUpgrade internal `angular.module()` method was
renamed to `angular.module_()` (to avoid a webpack bug).

Merging angular#29794 afterwards resulted in some broken tests, because it
still used the old `angular.module()` method name. (The PR had been
tested on CI against a revision that did not contain the rename.)

This commit fixes the broken tests by renaming the remaining occurrences
of `angular.module()`.
gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 25, 2019
With angular#30058, the ngUpgrade internal `angular.module()` method wa
renamed to `angular.module_()` (to avoid a webpack bug).

Merging angular#29794 afterwards resulted in some broken tests, because it
still used the old `angular.module()` method name. (The PR had been
tested on CI against a revision that did not contain the rename.)

This commit fixes the broken tests by renaming the remaining occurrences
of `angular.module()`.
AndrewKushnir pushed a commit that referenced this pull request Apr 25, 2019
With #30058, the ngUpgrade internal `angular.module()` method was
renamed to `angular.module_()` (to avoid a webpack bug).

Merging #29794 afterwards resulted in some broken tests, because it
still used the old `angular.module()` method name. (The PR had been
tested on CI against a revision that did not contain the rename.)

This commit fixes the broken tests by renaming the remaining occurrences
of `angular.module()`.

PR Close #30126
AndrewKushnir pushed a commit that referenced this pull request Apr 25, 2019
) (#30107)

When targeting ES2015 (as is the default in cli@8), `const` is not
downleveled to `var` and thus declaring `const module` throws an error
due to webpack wrapping the code in a function call with a `module`
argument (even when compiling for the `web` environment).

Related: webpack/webpack#7369

Fixes #30050

PR Close #30058

PR Close #30107
AndrewKushnir pushed a commit that referenced this pull request Apr 25, 2019
With #30058, the ngUpgrade internal `angular.module()` method wa
renamed to `angular.module_()` (to avoid a webpack bug).

Merging #29794 afterwards resulted in some broken tests, because it
still used the old `angular.module()` method name. (The PR had been
tested on CI against a revision that did not contain the rename.)

This commit fixes the broken tests by renaming the remaining occurrences
of `angular.module()`.

PR Close #30107
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
…ular#30058)

When targeting ES2015 (as is the default in cli@8), `const` is not
downleveled to `var` and thus declaring `const module` throws an error
due to webpack wrapping the code in a function call with a `module`
argument (even when compiling for the `web` environment).

Related: webpack/webpack#7369

Fixes angular#30050

PR Close angular#30058
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
…lar#30126)

With angular#30058, the ngUpgrade internal `angular.module()` method was
renamed to `angular.module_()` (to avoid a webpack bug).

Merging angular#29794 afterwards resulted in some broken tests, because it
still used the old `angular.module()` method name. (The PR had been
tested on CI against a revision that did not contain the rename.)

This commit fixes the broken tests by renaming the remaining occurrences
of `angular.module()`.

PR Close angular#30126
@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 15, 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: upgrade Issues related to AngularJS → Angular upgrade APIs cla: yes effort1: hours merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note refactoring Issue that involves refactoring or code-cleanup risk: low target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Syntax error in @angular/upgrade package due to differential loading
8 participants