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

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

@devversion
Copy link
Member

devversion commented Feb 9, 2019

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

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 #25644.

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

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

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

@devversion devversion force-pushed the devversion:feat/no-longer-reexport-external-symbols branch 6 times, most recently from b8fc6f5 to dff5d81 Feb 9, 2019

@devversion

This comment has been minimized.

Copy link
Member Author

devversion commented Feb 9, 2019

@IgorMinar @alxhub @alexeagle This is just the change from the other PR but we split the changes into two parts (here is why). Can you review/re-approve this PR. It will be still blocked until #28594 is merged.

Note that this change needed some test updates because the symbol re-exports will be disabled by default. All of these test changes seem reasonable and I've also added more test cases with the new expected behavior (e.g. testing both possible behaviors)

The diff just looks huge, but if you disable whitespace, it should be very reduced and clear.

@IgorMinar
Copy link
Member

IgorMinar left a comment

Can you please document why the strictness guarantees that blaze enforces doesn't apply to bazel? (I actually don't know why and it seems a bit suspicious - @alexeagle is this right?)

@alexeagle

This comment has been minimized.

Copy link
Contributor

alexeagle commented Feb 10, 2019

I don't know if it's right, it surprised me too, I think Alex suggested it. If anything it might be a diff between google3 and other repos rather than between bazel and blaze

@devversion devversion force-pushed the devversion:feat/no-longer-reexport-external-symbols branch from dff5d81 to 556fa11 Feb 10, 2019

@devversion

This comment has been minimized.

Copy link
Member Author

devversion commented Feb 10, 2019

@IgorMinar I've documented this better in the commit description and in the comments. This is based on what @alxhub wrote here. Please have another look.

@devversion devversion force-pushed the devversion:feat/no-longer-reexport-external-symbols branch from 556fa11 to ca8ae90 Feb 11, 2019

@devversion devversion force-pushed the devversion:feat/no-longer-reexport-external-symbols branch from ca8ae90 to 2e79eec Feb 12, 2019

@alxhub

This comment has been minimized.

Copy link
Contributor

alxhub commented Feb 12, 2019

@IgorMinar @alexeagle correct, it's because in g3 we have AJD which checks the generated code in certain circumstances, whereas in most repos we only check strictdeps at the TS level, before emit.

@alxhub

alxhub approved these changes Feb 12, 2019

Show resolved Hide resolved packages/bazel/src/ng_module.bzl Outdated

@devversion devversion force-pushed the devversion:feat/no-longer-reexport-external-symbols branch from 2e79eec to e38bebd Feb 12, 2019

@alexeagle

This comment has been minimized.

Copy link
Contributor

alexeagle commented Feb 12, 2019

feat(compiler-cli): no longer re-export external symbols by default
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.

@devversion devversion force-pushed the devversion:feat/no-longer-reexport-external-symbols branch from e38bebd to 41a8fba Feb 12, 2019

@IgorMinar

This comment has been minimized.

@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Feb 13, 2019

merge-assistance: please check on the presubmit result once it's ready. this PR looks good to me.

@mhevery mhevery closed this in 91b7152 Feb 13, 2019

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.