build: add missing @babel/generator bazel dep to fix //packages/localize/src/tools/test:test #34974

IgorMinar · 2020-01-26T15:48:25Z

It's not clear how this ever worked (npm hoisting is a suspect though), but this dep is required because
one of the tests imports @babel/generator in the ts sources.

The lack of this dep is breaking builds on the master branch.

More discussion about this issue on Slack: https://angular-team.slack.com/archives/C07DT5M6V/p1579934766007500

…ize/src/tools/test:test It's not clear how this ever worked (npm hoisting is a suspect though), but this dep is required because one of the tests imports @babel/generator in the ts sources. The lack of this dep is breaking builds on the master branch. More discussion about this issue on Slack: https://angular-team.slack.com/archives/C07DT5M6V/p1579934766007500

mary-poppins · 2020-01-26T15:58:19Z

You can preview af97ced at https://pr34974-af97ced.ngbuilds.io/.

gkalpak · 2020-01-26T17:19:38Z

packages/localize/src/tools/test/BUILD.bazel

@@ -11,6 +11,7 @@ ts_library(
        "//packages/compiler",
        "//packages/localize",
        "//packages/localize/src/tools",
+        "@npm//@babel/generator",


Do we need to have these packages in package.json or is BUILD.bazel enough?

I think you are right. I added it as well as the @types/babel__generator package in a separate commit.

gregmagolan · 2020-01-26T18:47:16Z

There are quite a few @babel/generators in node_modules on master.

./@angular-devkit/build-angular/node_modules/@babel/core/node_modules/@babel/generator/package.json
./@angular-devkit/build-angular/node_modules/@babel/generator/package.json
./@angular-devkit/build-angular/node_modules/@babel/traverse/node_modules/@babel/generator/package.json
./@angular-devkit/build-angular/node_modules/@babel/helpers/node_modules/@babel/generator/package.json
./@babel/generator/package.json
./@babel/traverse/node_modules/@babel/generator/package.json

The test that is causing trouble transitively depends on @babel/core which transitively depends on @babel/generator so its import from @babel/generator would only work if the @babel/core dep @bable/generator was being hoisted to the root.... which on master that looks to be the case:

cat node_modules/@babel/generator/package.json
{
  "name": "@babel/generator",
  "version": "7.6.4",

But if it wasn't hoisted due to package.json changes that could break the test.

I don't think this PR is sufficient to fix the problem in all cases as it is possible that there will be no @npm//@babel/generator layed out.

The file that uses @babel/generator is packages/localize/src/tools/test/translate/source_files/source_file_utils_spec.ts. So I think the correct shape for the fix is:

to add @babel/generator and @npm//@babel/core to the test_lib target packages/localize/src/tools/test/BUILD.bazel. Its currently getting both transitively from //packages/localize/src/tools:tools but only working as @babel/generator from @bable/core is being hoisted to the top.
explicitely add @babel/generator to the root package.json so we're not depending on the package manager hoisting it to the top

gregmagolan · 2020-01-26T18:55:38Z

@IgorMinar Threw up a quick #34980 with my suggestion

Previously we just happened to be able to use it because of npm hoisting.

…rc/tools/test:test_lib Previously we relied on this package being hoisted and available, which is error prone and it would be just a matter of time before the build would break due rehoisting of deps upon future npm package.json changes.

IgorMinar · 2020-01-26T20:10:26Z

merge-assistance: global approval

IgorMinar · 2020-01-26T20:11:02Z

@gregmagolan George noticed the same thing, and I addressed it in follow up commits.

mary-poppins · 2020-01-26T20:21:29Z

You can preview a8565a7 at https://pr34974-a8565a7.ngbuilds.io/.

the test failures most likely result from the babel updates in the previous commit. it does look like we lost the file path from the error message, which is something that we should follow up no in a separate change.

It seems that we missed adding it before and listed all the localize fixes under 'ivy'.

mary-poppins · 2020-01-26T21:59:20Z

You can preview 6020c70 at https://pr34974-6020c70.ngbuilds.io/.

IgorMinar · 2020-01-26T22:37:20Z

packages/localize/src/tools/test/translate/source_files/es5_translate_plugin_spec.ts

@@ -140,9 +140,9 @@ describe('makeEs5Plugin', () => {
         expect(diagnostics.hasErrors).toBe(true);
         expect(diagnostics.messages[0]).toEqual({
           type: 'error',
-           message: '/app/dist/test.js: `$localize` called without any arguments.\n' +


@petebacondarwin this is unexpected and likely due to the @babel version update in 0a5a944 can you please look into why the path disappeared and fix it in a follow up PR if possible? I'd like to get this PR landed so that it doesn't block other PRs.

Not sure, yet, why the file path has disappeared. But the other changes to the error messages (i.e. the extra ^^^^ etc) are due to this commit in Babel: babel/babel@f1bc6c4

Ahah! The filename was removed in this commit: babel/babel@dcf7d89

…ize/src/tools/test:test (#34974) It's not clear how this ever worked (npm hoisting is a suspect though), but this dep is required because one of the tests imports @babel/generator in the ts sources. The lack of this dep is breaking builds on the master branch. More discussion about this issue on Slack: https://angular-team.slack.com/archives/C07DT5M6V/p1579934766007500 PR Close #34974

Previously we just happened to be able to use it because of npm hoisting. PR Close #34974

…rc/tools/test:test_lib (#34974) Previously we relied on this package being hoisted and available, which is error prone and it would be just a matter of time before the build would break due rehoisting of deps upon future npm package.json changes. PR Close #34974

the test failures most likely result from the babel updates in the previous commit. it does look like we lost the file path from the error message, which is something that we should follow up no in a separate change. PR Close #34974

It seems that we missed adding it before and listed all the localize fixes under 'ivy'. PR Close #34974

Previously we just happened to be able to use it because of npm hoisting. PR Close #34974

…rc/tools/test:test_lib (#34974) Previously we relied on this package being hoisted and available, which is error prone and it would be just a matter of time before the build would break due rehoisting of deps upon future npm package.json changes. PR Close #34974

the test failures most likely result from the babel updates in the previous commit. it does look like we lost the file path from the error message, which is something that we should follow up no in a separate change. PR Close #34974

It seems that we missed adding it before and listed all the localize fixes under 'ivy'. PR Close #34974

In angular#34974 the top level dependency on `@babel/core` was bumped to 7.8.3. This commit ensures that the package.json that gets included in the `@angular/localize` distributable is at the same version.

In #34974 the top level dependency on `@babel/core` was bumped to 7.8.3. This commit ensures that the package.json that gets included in the `@angular/localize` distributable is at the same version. PR Close #35008

…ize/src/tools/test:test (angular#34974) It's not clear how this ever worked (npm hoisting is a suspect though), but this dep is required because one of the tests imports @babel/generator in the ts sources. The lack of this dep is breaking builds on the master branch. More discussion about this issue on Slack: https://angular-team.slack.com/archives/C07DT5M6V/p1579934766007500 PR Close angular#34974

…r#34974) Previously we just happened to be able to use it because of npm hoisting. PR Close angular#34974

…rc/tools/test:test_lib (angular#34974) Previously we relied on this package being hoisted and available, which is error prone and it would be just a matter of time before the build would break due rehoisting of deps upon future npm package.json changes. PR Close angular#34974

the test failures most likely result from the babel updates in the previous commit. it does look like we lost the file path from the error message, which is something that we should follow up no in a separate change. PR Close angular#34974

It seems that we missed adding it before and listed all the localize fixes under 'ivy'. PR Close angular#34974

In angular#34974 the top level dependency on `@babel/core` was bumped to 7.8.3. This commit ensures that the package.json that gets included in the `@angular/localize` distributable is at the same version. PR Close angular#35008

angular-automatic-lock-bot · 2020-02-27T16:08:16Z

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.}

IgorMinar requested review from a team as code owners January 26, 2020 15:48

googlebot added the cla: yes label Jan 26, 2020

IgorMinar changed the title ~~build: add missing @babel/generator bazel dep to fix //packages/loca…~~ build: add missing @babel/generator bazel dep to fix //packages/localize/src/tools/test:test Jan 26, 2020

IgorMinar added area: build & ci Related the build and CI infrastructure of the project target: patch This PR is targeted for the next patch release labels Jan 26, 2020

ngbot bot modified the milestone: needsTriage Jan 26, 2020

IgorMinar added the action: merge The PR is ready for merge by the caretaker label Jan 26, 2020

gkalpak approved these changes Jan 26, 2020

View reviewed changes

gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Jan 26, 2020

gregmagolan mentioned this pull request Jan 26, 2020

build: add missing @babel/generator bazel dep to fix //packages/localize/src/tools/test:test #34980

Closed

build: explicitly define @babel/generator dep in package.json

0a5a944

Previously we just happened to be able to use it because of npm hoisting.

IgorMinar requested a review from a team as a code owner January 26, 2020 20:04

IgorMinar added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 26, 2020

ngbot bot added the action: merge The PR is ready for merge by the caretaker label Jan 26, 2020

IgorMinar added 2 commits January 26, 2020 13:43

test(localize): fix broken localize/translate tests

5202c1e

the test failures most likely result from the babel updates in the previous commit. it does look like we lost the file path from the error message, which is something that we should follow up no in a separate change.

test: add 'localize' as an allowed commit message scope

6020c70

It seems that we missed adding it before and listed all the localize fixes under 'ivy'.

IgorMinar commented Jan 26, 2020

View reviewed changes

IgorMinar added a commit that referenced this pull request Jan 26, 2020

build: explicitly define @babel/generator dep in package.json (#34974)

789706f

Previously we just happened to be able to use it because of npm hoisting. PR Close #34974

IgorMinar added a commit that referenced this pull request Jan 26, 2020

test: add 'localize' as an allowed commit message scope (#34974)

4a6093d

It seems that we missed adding it before and listed all the localize fixes under 'ivy'. PR Close #34974

IgorMinar closed this in a58034b Jan 26, 2020

IgorMinar added a commit that referenced this pull request Jan 26, 2020

build: explicitly define @babel/generator dep in package.json (#34974)

241f0ac

Previously we just happened to be able to use it because of npm hoisting. PR Close #34974

IgorMinar added a commit that referenced this pull request Jan 26, 2020

test: add 'localize' as an allowed commit message scope (#34974)

3d4b143

It seems that we missed adding it before and listed all the localize fixes under 'ivy'. PR Close #34974

petebacondarwin mentioned this pull request Jan 28, 2020

Babel error during localized bundle generation v9.0.0-rc.10 angular/angular-cli#16781

Closed

15 tasks

petebacondarwin mentioned this pull request Jan 28, 2020

build(localize): sync up babel dependencies #35008

Closed

sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 13, 2020

build: explicitly define @babel/generator dep in package.json (angula…

2f7c793

…r#34974) Previously we just happened to be able to use it because of npm hoisting. PR Close angular#34974

sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 13, 2020

test: add 'localize' as an allowed commit message scope (angular#34974)

356c929

It seems that we missed adding it before and listed all the localize fixes under 'ivy'. PR Close angular#34974

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add missing @babel/generator bazel dep to fix //packages/localize/src/tools/test:test #34974

build: add missing @babel/generator bazel dep to fix //packages/localize/src/tools/test:test #34974

IgorMinar commented Jan 26, 2020 •

edited

mary-poppins commented Jan 26, 2020

gkalpak Jan 26, 2020

IgorMinar Jan 26, 2020

gregmagolan commented Jan 26, 2020 •

edited

gregmagolan commented Jan 26, 2020

IgorMinar commented Jan 26, 2020

IgorMinar commented Jan 26, 2020

mary-poppins commented Jan 26, 2020

mary-poppins commented Jan 26, 2020

IgorMinar Jan 26, 2020 •

edited

petebacondarwin Jan 27, 2020

petebacondarwin Jan 27, 2020 •

edited

angular-automatic-lock-bot bot commented Feb 27, 2020

build: add missing @babel/generator bazel dep to fix //packages/localize/src/tools/test:test #34974

build: add missing @babel/generator bazel dep to fix //packages/localize/src/tools/test:test #34974

Conversation

IgorMinar commented Jan 26, 2020 • edited

mary-poppins commented Jan 26, 2020

gkalpak Jan 26, 2020

Choose a reason for hiding this comment

IgorMinar Jan 26, 2020

Choose a reason for hiding this comment

gregmagolan commented Jan 26, 2020 • edited

gregmagolan commented Jan 26, 2020

IgorMinar commented Jan 26, 2020

IgorMinar commented Jan 26, 2020

mary-poppins commented Jan 26, 2020

mary-poppins commented Jan 26, 2020

IgorMinar Jan 26, 2020 • edited

Choose a reason for hiding this comment

petebacondarwin Jan 27, 2020

Choose a reason for hiding this comment

petebacondarwin Jan 27, 2020 • edited

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Feb 27, 2020

IgorMinar commented Jan 26, 2020 •

edited

gregmagolan commented Jan 26, 2020 •

edited

IgorMinar Jan 26, 2020 •

edited

petebacondarwin Jan 27, 2020 •

edited