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(bazel): do not use manifest paths for generated imports within compilation unit #35841

Conversation

devversion
Copy link
Member

@devversion devversion commented Mar 3, 2020

Currently, the ng_module rule incorrectly uses manifest paths for
generated imports from the Angular compiler.

This breaks packaging as prodmode output (i.e. esnext) is copied in
various targets (es5 and es2015) to the npm package output. Example:

node_modules/my-pkg/es2015/imports/public-api.js

import * as i1 from "angular/packages/bazel/test/ng_package/example/imports/second";

The above import is invalid in NPM and leaks information about the source project
structure. The import should be relative:

import * as i1 from "./second";

The imports can, and should be relative so that the files of the same compilation unit are self-contained and do not rely on custom module resolution.

@devversion devversion force-pushed the fix/bazel-ng-package-generated-imports branch 2 times, most recently from 36ba941 to fe1fe4b Compare March 3, 2020 20:56
@devversion devversion added area: bazel Issues related to the published `@angular/bazel` build rules hotlist: components team Related to Angular CDK or Angular Material action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Mar 3, 2020
@ngbot ngbot bot modified the milestone: needsTriage Mar 3, 2020
@devversion devversion marked this pull request as ready for review March 3, 2020 20:57
@pullapprove pullapprove bot requested review from gkalpak and kyliau March 3, 2020 20:57
@gregmagolan gregmagolan self-requested a review March 3, 2020 21:12
Copy link
Contributor

@gregmagolan gregmagolan left a comment

Choose a reason for hiding this comment

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

LGTM. This code is much too complicated. We will be able to replace this with ts_library with angular as a plugin in the future once we can drop
ViewEngine support.

@devversion
Copy link
Member Author

Yep. Totally agreed. And yes! I'm waiting for that to happen. The current system is too complicated. I tried to improve this by adding comments. Also the fact that we rely on summaries, but not on metadata files, makes ngc-wrapped even more complicated.

@kyliau
Copy link
Contributor

kyliau commented Mar 3, 2020

@devversion devversion force-pushed the fix/bazel-ng-package-generated-imports branch from fe1fe4b to 38df841 Compare March 4, 2020 12:52
@kyliau
Copy link
Contributor

kyliau commented Mar 4, 2020

@devversion devversion force-pushed the fix/bazel-ng-package-generated-imports branch from 38df841 to 531cc12 Compare March 4, 2020 19:17
@devversion devversion force-pushed the fix/bazel-ng-package-generated-imports branch from 531cc12 to 3d6ecfb Compare March 4, 2020 19:20
@kyliau
Copy link
Contributor

kyliau commented Mar 4, 2020

…mpilation unit

Currently, the `ng_module` rule incorrectly uses manifest paths for
generated imports from the Angular compiler.

This breaks packaging as prodmode output (i.e. `esnext`) is copied in
various targets (`es5` and `es2015`) to the npm package output.

e.g. imports are generated like:

_node_modules/my-pkg/es2015/imports/public-api.js_
```ts
import * as i1 from "angular/packages/bazel/test/ng_package/example/imports/second";
```

while it should be actually:

```ts
import * as i1 from "./second";
```

The imports can, and should be relative so that the files are
self-contained and do not rely on custom module resolution.
@devversion devversion force-pushed the fix/bazel-ng-package-generated-imports branch from 3d6ecfb to 36a09a2 Compare March 5, 2020 19:43
@kyliau
Copy link
Contributor

kyliau commented Mar 5, 2020

@kyliau
Copy link
Contributor

kyliau commented Mar 6, 2020

@devversion devversion changed the title fix(bazel): do not use manifest paths for generated imports fix(bazel): do not use manifest paths for generated imports within compilation unit Mar 6, 2020
@devversion devversion 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 Mar 6, 2020
@matsko matsko closed this in 68bebd6 Mar 6, 2020
matsko pushed a commit that referenced this pull request Mar 6, 2020
…mpilation unit (#35841)

Currently, the `ng_module` rule incorrectly uses manifest paths for
generated imports from the Angular compiler.

This breaks packaging as prodmode output (i.e. `esnext`) is copied in
various targets (`es5` and `es2015`) to the npm package output.

e.g. imports are generated like:

_node_modules/my-pkg/es2015/imports/public-api.js_
```ts
import * as i1 from "angular/packages/bazel/test/ng_package/example/imports/second";
```

while it should be actually:

```ts
import * as i1 from "./second";
```

The imports can, and should be relative so that the files are
self-contained and do not rely on custom module resolution.

PR Close #35841
matsko pushed a commit that referenced this pull request Mar 6, 2020
…mpilation unit (#35841)

Currently, the `ng_module` rule incorrectly uses manifest paths for
generated imports from the Angular compiler.

This breaks packaging as prodmode output (i.e. `esnext`) is copied in
various targets (`es5` and `es2015`) to the npm package output.

e.g. imports are generated like:

_node_modules/my-pkg/es2015/imports/public-api.js_
```ts
import * as i1 from "angular/packages/bazel/test/ng_package/example/imports/second";
```

while it should be actually:

```ts
import * as i1 from "./second";
```

The imports can, and should be relative so that the files are
self-contained and do not rely on custom module resolution.

PR Close #35841
@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 Apr 6, 2020
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: bazel Issues related to the published `@angular/bazel` build rules cla: yes hotlist: components team Related to Angular CDK or Angular Material target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants