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(compiler): allow disabling external symbol factory reexports #28594

Closed
wants to merge 1 commit into
base: master
from

Conversation

@devversion
Copy link
Member

devversion commented Feb 7, 2019

See individual commits for description of changes.

@devversion devversion self-assigned this Feb 7, 2019

@devversion devversion requested review from angular/fw-compiler as code owners Feb 7, 2019

@googlebot googlebot added the cla: yes label Feb 7, 2019

@devversion devversion force-pushed the devversion:fix/compiler-disable-external-symbol-factory-reexports branch from 275ab59 to 52c1ed2 Feb 7, 2019

@ngbot ngbot bot added this to the needsTriage milestone Feb 7, 2019

@devversion devversion assigned alexeagle and alxhub and unassigned devversion Feb 7, 2019

@devversion devversion force-pushed the devversion:fix/compiler-disable-external-symbol-factory-reexports branch from e6e0344 to a57b27f Feb 7, 2019

@alexeagle

This comment has been minimized.

Copy link
Contributor

alexeagle commented Feb 7, 2019

test failure looks legit?

Failures:
1) ngc transformer command-line compile ngfactory files should be able to compile multiple libraries with summaries
  Message:
    Error: Expected lib2_built/module.ngfactory.js to be emitted (outDir: /b/f/w/_tmp/8738305b11d10557227ccf1484986acd/tmp.398296/node_modules)
  Stack:
    Error: Expected lib2_built/module.ngfactory.js to be emitted (outDir: /b/f/w/_tmp/8738305b11d10557227ccf1484986acd/tmp.398296/node_modules)
        at shouldExist (packages/compiler-cli/test/ngc_spec.ts:24:13)
Show resolved Hide resolved packages/bazel/src/ng_module.bzl Outdated
@devversion

This comment has been minimized.

Copy link
Member Author

devversion commented Feb 7, 2019

@alexeagle Yeah test failure looks legit. I'm looking into it. edit: test fixed.

@devversion devversion force-pushed the devversion:fix/compiler-disable-external-symbol-factory-reexports branch 2 times, most recently from d5ddbf7 to 9320ad8 Feb 8, 2019

@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Feb 8, 2019

is this still WIP?

@devversion

This comment has been minimized.

Copy link
Member Author

devversion commented Feb 8, 2019

It's actually ready for review 😄

@IgorMinar @alxhub Can you please look at my questions/points below?

  1. This is technically a change in behavior since we by default disable symbol re-exports within NGC. So this shouldn't go into patch for now, right?

  2. Should I change the refactor(compiler) message to feat(compiler) because this basically a new flag for the AotCompiler, but I wasn't sure if it's worth to be shown as feat in the changelog

  3. For NGC we disable symbol re-exports by default now, but for @angular/compiler I kept the changes backwards-compatible by making the option default to true. see here. It would cleaner to just default to false (similar to NGC), but I could imagine that being considered as a breaking change. thoughts?

@alexeagle

This comment has been minimized.

Copy link
Contributor

alexeagle commented Feb 8, 2019

@alxhub

alxhub approved these changes Feb 8, 2019

Copy link
Contributor

alxhub left a comment

Code-wise, this all looks good to me. To answer your questions.

  1. This is technically a change in behavior since we by default disable symbol re-exports within NGC. So this shouldn't go into patch for now, right?

I think we should land the change in both master and patch with the default unchanged (always produce re-exports in every case, like we did before) and with the flag removed from parsing, so users can't change it. Then, in a separate PR we can introduce the flag on master only, with default values that disable re-exports in the non-Blaze case.

  1. Should I change the refactor(compiler) message to feat(compiler) because this basically a new flag for the AotCompiler, but I wasn't sure if it's worth to be shown as feat in the changelog.

No, leave it as refactor but remove the publicly visible part of the change (introduction of the flag). That should go in a separate master-only commit that is a feat.

  1. For NGC we disable symbol re-exports by default now, but for @angular/compiler I kept the changes backwards-compatible by making the option default to true. [see here (https://github.com//pull/28594/files#diff-55c632657b5b6ecc7a41905de326e901R25). It would cleaner to just default to false (similar to NGC), but I could imagine that being considered as a breaking change. thoughts?

As above.

Show resolved Hide resolved packages/bazel/src/ng_module.bzl Outdated
refactor(compiler): allow disabling external symbol factory reexports
Currently external static symbols which are referenced by AOT
compiler generated code, will be re-exported in the corresponding
`.ngfactory` files.

This way of handling the symbol resolution has been introduced in
favor of avoding dynamically generated module dependencies. This
behavior therefore avoids any strict dependency failures.

Read more about a particular scenario here: #25644 (comment)

Now with `ngtsc`, this behavior has changed since `ngtsc` just
introduces these module dependencies in order to properly reference
the external symbol from its original location (also eliminating the need
for factories). Similarly we should provide a way to use the same
behavior with `ngc` because the downside of using the re-exported symbol
resolution is that user-code transformations (e.g. the `ngInjectableDef`
metadata which is added to the user source code), can resolve external
symbols to previous factory symbol re-exports. This is a critical issue
because it means that the actual JIT code references factory files in order
to access external symbols. This means that the generated output cannot
shipped to NPM without shipping the referenced factory files.

A specific example has been reported here: #25644 (comment)

@devversion devversion force-pushed the devversion:fix/compiler-disable-external-symbol-factory-reexports branch from 9320ad8 to 883fb4b Feb 9, 2019

devversion added a commit to devversion/angular that referenced this pull request Feb 9, 2019

feat(compiler-cli): no longer re-export external symbols by default
With angular#28594 we refactored the `@angular/compiler` slightly to
allow opting out from external symbol re-exports which are
enabled by default.

Since symbol re-exports only benefit projects which have a
very strict dependency enforcement, external symbols should
not be re-exported by default as this could grow the size of
factory files and cause unexpected behavior with Angular's
AOT symbol resolving (e.g. see: angular#25644).

Additionally "ngtsc" also does not provide any way of using
external symbol re-exports, so this means that NGC partially
matches the behavior of "ngtsc" then (unless explicitly opted-out).

Internally at Google, symbol re-exports need to be still enabled
since Google has a very strict dependency enforcement (even of
produced JavaScript output), so the `ng_module` Bazel rule will
enable the symbol re-exports by default when running within Blaze.

Fixes angular#25644.

devversion added a commit to devversion/angular that referenced this pull request Feb 9, 2019

feat(compiler-cli): no longer re-export external symbols by default
With angular#28594 we refactored the `@angular/compiler` slightly to
allow opting out from external symbol re-exports which are
enabled by default.

Since symbol re-exports only benefit projects which have a
very strict dependency enforcement, external symbols should
not be re-exported by default as this could grow the size of
factory files and cause unexpected behavior with Angular's
AOT symbol resolving (e.g. see: angular#25644).

Additionally "ngtsc" also does not provide any way of using
external symbol re-exports, so this means that NGC partially
matches the behavior of "ngtsc" then (unless explicitly opted-out).

Internally at Google, symbol re-exports need to be still enabled
since Google has a very strict dependency enforcement (even of
produced JavaScript output), so the `ng_module` Bazel rule will
enable the symbol re-exports by default when running within Blaze.

Fixes angular#25644.
@devversion

This comment has been minimized.

Copy link
Member Author

devversion commented Feb 9, 2019

Caretaker: The saucelabs job timed out due to flakiness. There is a presubmit here, but not sure if it's outdated now.

devversion added a commit to devversion/angular that referenced this pull request Feb 9, 2019

feat(compiler-cli): no longer re-export external symbols by default
With angular#28594 we refactored the `@angular/compiler` slightly to
allow opting out from external symbol re-exports which are
enabled by default.

Since symbol re-exports only benefit projects which have a
very strict dependency enforcement, external symbols should
not be re-exported by default as this could grow the size of
factory files and cause unexpected behavior with Angular's
AOT symbol resolving (e.g. see: angular#25644).

Additionally "ngtsc" also does not provide any way of using
external symbol re-exports, so this means that NGC partially
matches the behavior of "ngtsc" then (unless explicitly opted-out).

Internally at Google, symbol re-exports need to be still enabled
since Google has a very strict dependency enforcement (even of
produced JavaScript output), so the `ng_module` Bazel rule will
enable the symbol re-exports by default when running within Blaze.

Fixes angular#25644.

devversion added a commit to devversion/angular that referenced this pull request Feb 10, 2019

feat(compiler-cli): no longer re-export external symbols by default
With angular#28594 we refactored the `@angular/compiler` slightly to
allow opting out from external symbol re-exports which are
enabled by default.

Since symbol re-exports only benefit projects which have a
very strict dependency enforcement, external symbols should
not be re-exported by default as this could grow the size of
factory files and cause unexpected behavior with Angular's
AOT symbol resolving (e.g. see: angular#25644).

Note that the common strict dependency enforcement for source
files does still work with external symbol re-exports disabled,
but there are also strict dependency checks that enforce strict
module dependencies also for _generated files_ (such as the
ngfactory files). This is how Google3 manages it's dependencies
and therefore external symbol re-exports need to be enabled within
Google3.

Also "ngtsc" also does not provide any way of using external symbol
re-exports, so this means that with this change, NGC can partially
match the behavior of "ngtsc" then (unless explicitly opted-out).

As mentioned before, internally at Google symbol re-exports need to
be still enabled, so the `ng_module` Bazel rule will enable the symbol
re-exports by default when running within Blaze.

Fixes angular#25644.

mhevery added a commit that referenced this pull request Feb 11, 2019

refactor(compiler): allow disabling external symbol factory reexports (
…#28594)

Currently external static symbols which are referenced by AOT
compiler generated code, will be re-exported in the corresponding
`.ngfactory` files.

This way of handling the symbol resolution has been introduced in
favor of avoding dynamically generated module dependencies. This
behavior therefore avoids any strict dependency failures.

Read more about a particular scenario here: #25644 (comment)

Now with `ngtsc`, this behavior has changed since `ngtsc` just
introduces these module dependencies in order to properly reference
the external symbol from its original location (also eliminating the need
for factories). Similarly we should provide a way to use the same
behavior with `ngc` because the downside of using the re-exported symbol
resolution is that user-code transformations (e.g. the `ngInjectableDef`
metadata which is added to the user source code), can resolve external
symbols to previous factory symbol re-exports. This is a critical issue
because it means that the actual JIT code references factory files in order
to access external symbols. This means that the generated output cannot
shipped to NPM without shipping the referenced factory files.

A specific example has been reported here: #25644 (comment)

PR Close #28594

@mhevery mhevery closed this in 872a365 Feb 11, 2019

devversion added a commit to devversion/angular that referenced this pull request Feb 11, 2019

feat(compiler-cli): no longer re-export external symbols by default
With angular#28594 we refactored the `@angular/compiler` slightly to
allow opting out from external symbol re-exports which are
enabled by default.

Since symbol re-exports only benefit projects which have a
very strict dependency enforcement, external symbols should
not be re-exported by default as this could grow the size of
factory files and cause unexpected behavior with Angular's
AOT symbol resolving (e.g. see: angular#25644).

Additionally "ngtsc" also does not provide any way of using
external symbol re-exports, so this means that NGC partially
matches the behavior of "ngtsc" then (unless explicitly opted-out).

Internally at Google, symbol re-exports need to be still enabled
since Google has a very strict dependency enforcement (even of
produced JavaScript output), so the `ng_module` Bazel rule will
enable the symbol re-exports by default when running within Blaze.

Fixes angular#25644.

devversion added a commit to devversion/angular that referenced this pull request Feb 12, 2019

feat(compiler-cli): no longer re-export external symbols by default
With angular#28594 we refactored the `@angular/compiler` slightly to
allow opting out from external symbol re-exports which are
enabled by default.

Since symbol re-exports only benefit projects which have a
very strict dependency enforcement, external symbols should
not be re-exported by default as this could grow the size of
factory files and cause unexpected behavior with Angular's
AOT symbol resolving (e.g. see: angular#25644).

Additionally "ngtsc" also does not provide any way of using
external symbol re-exports, so this means that NGC partially
matches the behavior of "ngtsc" then (unless explicitly opted-out).

Internally at Google, symbol re-exports need to be still enabled
since Google has a very strict dependency enforcement (even of
produced JavaScript output), so the `ng_module` Bazel rule will
enable the symbol re-exports by default when running within Blaze.

Fixes angular#25644.

devversion added a commit to devversion/angular that referenced this pull request Feb 12, 2019

feat(compiler-cli): no longer re-export external symbols by default
With angular#28594 we refactored the `@angular/compiler` slightly to
allow opting out from external symbol re-exports which are
enabled by default.

Since symbol re-exports only benefit projects which have a
very strict dependency enforcement, external symbols should
not be re-exported by default as this could grow the size of
factory files and cause unexpected behavior with Angular's
AOT symbol resolving (e.g. see: angular#25644).

Additionally "ngtsc" also does not provide any way of using
external symbol re-exports, so this means that NGC partially
matches the behavior of "ngtsc" then (unless explicitly opted-out).

Internally at Google, symbol re-exports need to be still enabled
since Google has a very strict dependency enforcement (even of
produced JavaScript output), so the `ng_module` Bazel rule will
enable the symbol re-exports by default when running within Blaze.

Fixes angular#25644.

devversion added a commit to devversion/angular that referenced this pull request Feb 12, 2019

feat(compiler-cli): no longer re-export external symbols by default
With angular#28594 we refactored the `@angular/compiler` slightly to
allow opting out from external symbol re-exports which are
enabled by default.

Since symbol re-exports only benefit projects which have a
very strict dependency enforcement, external symbols should
not be re-exported by default as this could grow the size of
factory files and cause unexpected behavior with Angular's
AOT symbol resolving (e.g. see: angular#25644).

Note that the common strict dependency enforcement for source
files does still work with external symbol re-exports disabled,
but there are also strict dependency checks that enforce strict
module dependencies also for _generated files_ (such as the
ngfactory files). This is how Google3 manages it's dependencies
and therefore external symbol re-exports need to be enabled within
Google3.

Also "ngtsc" also does not provide any way of using external symbol
re-exports, so this means that with this change, NGC can partially
match the behavior of "ngtsc" then (unless explicitly opted-out).

As mentioned before, internally at Google symbol re-exports need to
be still enabled, so the `ng_module` Bazel rule will enable the symbol
re-exports by default when running within Blaze.

Fixes angular#25644.

mhevery added a commit that referenced this pull request Feb 13, 2019

feat(compiler-cli): no longer re-export external symbols by default (#…
…28633)

With #28594 we refactored the `@angular/compiler` slightly to
allow opting out from external symbol re-exports which are
enabled by default.

Since symbol re-exports only benefit projects which have a
very strict dependency enforcement, external symbols should
not be re-exported by default as this could grow the size of
factory files and cause unexpected behavior with Angular's
AOT symbol resolving (e.g. see: #25644).

Note that the common strict dependency enforcement for source
files does still work with external symbol re-exports disabled,
but there are also strict dependency checks that enforce strict
module dependencies also for _generated files_ (such as the
ngfactory files). This is how Google3 manages it's dependencies
and therefore external symbol re-exports need to be enabled within
Google3.

Also "ngtsc" also does not provide any way of using external symbol
re-exports, so this means that with this change, NGC can partially
match the behavior of "ngtsc" then (unless explicitly opted-out).

As mentioned before, internally at Google symbol re-exports need to
be still enabled, so the `ng_module` Bazel rule will enable the symbol
re-exports by default when running within Blaze.

Fixes #25644.

PR Close #28633
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.