-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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(bazel): transform generated shims (in Ivy) with tsickle #35848
Conversation
I intend to run a global presubmit on this PR. |
20839f4
to
76d5117
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for a very detailed description of the problem and the fix @alxhub 👍
I've left a couple comments, could you plz have a look? Thank you.
76d5117
to
6dc7c2a
Compare
Presubmit with the minor changes. |
Currently, when Angular code is built with Bazel and with Ivy, generated factory shims (.ngfactory files) are not processed via the majority of tsickle's transforms. This is a subtle effect of the build infrastructure, but it boils down to a TsickleHost method `shouldSkipTsickleProcessing`. For ngc_wrapped builds (Bazel + Angular), this method is defined in the `@bazel/typescript` (aka bazel rules_typescript) implementation of `CompilerHost`. The default behavior is to skip tsickle processing for files which are not present in the original `srcs[]` of the build rule. In Angular's case, this includes all generated shim files. For View Engine factories this is probably desirable as they're quite complex and they've never been tested with tsickle. Ivy factories however are smaller and very straightforward, and it makes sense to treat them like any other output. This commit adjusts two independent implementations of `shouldSkipTsickleProcessing` to enable transformation of Ivy shims: * in `@angular/bazel` aka ngc_wrapped, the upstream `@bazel/typescript` `CompilerHost` is patched to treat .ngfactory files the same as their original source file, with respect to tsickle processing. It is currently not possible to test this change as we don't have any test that inspects tsickle output with bazel. It will be extensively tested in g3. * in `ngc`, Angular's own implementation is adjusted to allow for the processing of shims when compiling with Ivy. This enables a unit test to be written to validate the correct behavior of tsickle when given a host that's appropriately configured to process factory shims. For ngtsc-as-a-plugin, a similar fix will need to be submitted upstream in tsc_wrapped.
6dc7c2a
to
5ed75be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for updates @alxhub! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - thanks!!
@alxhub consider making this a master&patch PR |
Thanks, Alex! |
…ngular#35848)" This reverts commit 9ff9a07.
…#35848) Currently, when Angular code is built with Bazel and with Ivy, generated factory shims (.ngfactory files) are not processed via the majority of tsickle's transforms. This is a subtle effect of the build infrastructure, but it boils down to a TsickleHost method `shouldSkipTsickleProcessing`. For ngc_wrapped builds (Bazel + Angular), this method is defined in the `@bazel/typescript` (aka bazel rules_typescript) implementation of `CompilerHost`. The default behavior is to skip tsickle processing for files which are not present in the original `srcs[]` of the build rule. In Angular's case, this includes all generated shim files. For View Engine factories this is probably desirable as they're quite complex and they've never been tested with tsickle. Ivy factories however are smaller and very straightforward, and it makes sense to treat them like any other output. This commit adjusts two independent implementations of `shouldSkipTsickleProcessing` to enable transformation of Ivy shims: * in `@angular/bazel` aka ngc_wrapped, the upstream `@bazel/typescript` `CompilerHost` is patched to treat .ngfactory files the same as their original source file, with respect to tsickle processing. It is currently not possible to test this change as we don't have any test that inspects tsickle output with bazel. It will be extensively tested in g3. * in `ngc`, Angular's own implementation is adjusted to allow for the processing of shims when compiling with Ivy. This enables a unit test to be written to validate the correct behavior of tsickle when given a host that's appropriately configured to process factory shims. For ngtsc-as-a-plugin, a similar fix will need to be submitted upstream in tsc_wrapped. PR Close angular#35848
Currently, when Angular code is built with Bazel and with Ivy, generated factory shims (.ngfactory files) are not processed via the majority of tsickle's transforms. This is a subtle effect of the build infrastructure, but it boils down to a TsickleHost method `shouldSkipTsickleProcessing`. For ngc_wrapped builds (Bazel + Angular), this method is defined in the `@bazel/typescript` (aka bazel rules_typescript) implementation of `CompilerHost`. The default behavior is to skip tsickle processing for files which are not present in the original `srcs[]` of the build rule. In Angular's case, this includes all generated shim files. For View Engine factories this is probably desirable as they're quite complex and they've never been tested with tsickle. Ivy factories however are smaller and very straightforward, and it makes sense to treat them like any other output. This commit adjusts two independent implementations of `shouldSkipTsickleProcessing` to enable transformation of Ivy shims: * in `@angular/bazel` aka ngc_wrapped, the upstream `@bazel/typescript` `CompilerHost` is patched to treat .ngfactory files the same as their original source file, with respect to tsickle processing. It is currently not possible to test this change as we don't have any test that inspects tsickle output with bazel. It will be extensively tested in g3. * in `ngc`, Angular's own implementation is adjusted to allow for the processing of shims when compiling with Ivy. This enables a unit test to be written to validate the correct behavior of tsickle when given a host that's appropriately configured to process factory shims. For ngtsc-as-a-plugin, a similar fix will need to be submitted upstream in tsc_wrapped. PR Close angular#35848
Currently, when Angular code is built with Bazel and with Ivy, generated factory shims (.ngfactory files) are not processed via the majority of tsickle's transforms. This is a subtle effect of the build infrastructure, but it boils down to a TsickleHost method `shouldSkipTsickleProcessing`. For ngc_wrapped builds (Bazel + Angular), this method is defined in the `@bazel/typescript` (aka bazel rules_typescript) implementation of `CompilerHost`. The default behavior is to skip tsickle processing for files which are not present in the original `srcs[]` of the build rule. In Angular's case, this includes all generated shim files. For View Engine factories this is probably desirable as they're quite complex and they've never been tested with tsickle. Ivy factories however are smaller and very straightforward, and it makes sense to treat them like any other output. This commit adjusts two independent implementations of `shouldSkipTsickleProcessing` to enable transformation of Ivy shims: * in `@angular/bazel` aka ngc_wrapped, the upstream `@bazel/typescript` `CompilerHost` is patched to treat .ngfactory files the same as their original source file, with respect to tsickle processing. It is currently not possible to test this change as we don't have any test that inspects tsickle output with bazel. It will be extensively tested in g3. * in `ngc`, Angular's own implementation is adjusted to allow for the processing of shims when compiling with Ivy. This enables a unit test to be written to validate the correct behavior of tsickle when given a host that's appropriately configured to process factory shims. For ngtsc-as-a-plugin, a similar fix will need to be submitted upstream in tsc_wrapped. PR Close angular#35848
Currently, when Angular code is built with Bazel and with Ivy, generated factory shims (.ngfactory files) are not processed via the majority of tsickle's transforms. This is a subtle effect of the build infrastructure, but it boils down to a TsickleHost method `shouldSkipTsickleProcessing`. For ngc_wrapped builds (Bazel + Angular), this method is defined in the `@bazel/typescript` (aka bazel rules_typescript) implementation of `CompilerHost`. The default behavior is to skip tsickle processing for files which are not present in the original `srcs[]` of the build rule. In Angular's case, this includes all generated shim files. For View Engine factories this is probably desirable as they're quite complex and they've never been tested with tsickle. Ivy factories however are smaller and very straightforward, and it makes sense to treat them like any other output. This commit adjusts two independent implementations of `shouldSkipTsickleProcessing` to enable transformation of Ivy shims: * in `@angular/bazel` aka ngc_wrapped, the upstream `@bazel/typescript` `CompilerHost` is patched to treat .ngfactory files the same as their original source file, with respect to tsickle processing. It is currently not possible to test this change as we don't have any test that inspects tsickle output with bazel. It will be extensively tested in g3. * in `ngc`, Angular's own implementation is adjusted to allow for the processing of shims when compiling with Ivy. This enables a unit test to be written to validate the correct behavior of tsickle when given a host that's appropriately configured to process factory shims. For ngtsc-as-a-plugin, a similar fix will need to be submitted upstream in tsc_wrapped. PR Close angular#35848
Currently, when Angular code is built with Bazel and with Ivy, generated factory shims (.ngfactory files) are not processed via the majority of tsickle's transforms. This is a subtle effect of the build infrastructure, but it boils down to a TsickleHost method `shouldSkipTsickleProcessing`. For ngc_wrapped builds (Bazel + Angular), this method is defined in the `@bazel/typescript` (aka bazel rules_typescript) implementation of `CompilerHost`. The default behavior is to skip tsickle processing for files which are not present in the original `srcs[]` of the build rule. In Angular's case, this includes all generated shim files. For View Engine factories this is probably desirable as they're quite complex and they've never been tested with tsickle. Ivy factories however are smaller and very straightforward, and it makes sense to treat them like any other output. This commit adjusts two independent implementations of `shouldSkipTsickleProcessing` to enable transformation of Ivy shims: * in `@angular/bazel` aka ngc_wrapped, the upstream `@bazel/typescript` `CompilerHost` is patched to treat .ngfactory files the same as their original source file, with respect to tsickle processing. It is currently not possible to test this change as we don't have any test that inspects tsickle output with bazel. It will be extensively tested in g3. * in `ngc`, Angular's own implementation is adjusted to allow for the processing of shims when compiling with Ivy. This enables a unit test to be written to validate the correct behavior of tsickle when given a host that's appropriately configured to process factory shims. For ngtsc-as-a-plugin, a similar fix will need to be submitted upstream in tsc_wrapped. PR Close angular#35848
Currently, when Angular code is built with Bazel and with Ivy, generated factory shims (.ngfactory files) are not processed via the majority of tsickle's transforms. This is a subtle effect of the build infrastructure, but it boils down to a TsickleHost method `shouldSkipTsickleProcessing`. For ngc_wrapped builds (Bazel + Angular), this method is defined in the `@bazel/typescript` (aka bazel rules_typescript) implementation of `CompilerHost`. The default behavior is to skip tsickle processing for files which are not present in the original `srcs[]` of the build rule. In Angular's case, this includes all generated shim files. For View Engine factories this is probably desirable as they're quite complex and they've never been tested with tsickle. Ivy factories however are smaller and very straightforward, and it makes sense to treat them like any other output. This commit adjusts two independent implementations of `shouldSkipTsickleProcessing` to enable transformation of Ivy shims: * in `@angular/bazel` aka ngc_wrapped, the upstream `@bazel/typescript` `CompilerHost` is patched to treat .ngfactory files the same as their original source file, with respect to tsickle processing. It is currently not possible to test this change as we don't have any test that inspects tsickle output with bazel. It will be extensively tested in g3. * in `ngc`, Angular's own implementation is adjusted to allow for the processing of shims when compiling with Ivy. This enables a unit test to be written to validate the correct behavior of tsickle when given a host that's appropriately configured to process factory shims. For ngtsc-as-a-plugin, a similar fix will need to be submitted upstream in tsc_wrapped. PR Close #35848 PR Close #35975
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Currently, when Angular code is built with Bazel and with Ivy, generated
factory shims (.ngfactory files) are not processed via the majority of
tsickle's transforms. This is a subtle effect of the build infrastructure,
but it boils down to a TsickleHost method
shouldSkipTsickleProcessing
.For ngc_wrapped builds (Bazel + Angular), this method is defined in the
@bazel/typescript
(aka bazel rules_typescript) implementation ofCompilerHost
. The default behavior is to skip tsickle processing for fileswhich are not present in the original
srcs[]
of the build rule. InAngular's case, this includes all generated shim files.
For View Engine factories this is probably desirable as they're quite
complex and they've never been tested with tsickle. Ivy factories however
are smaller and very straightforward, and it makes sense to treat them like
any other output.
This commit adjusts two independent implementations of
shouldSkipTsickleProcessing
to enable transformation of Ivy shims:in
@angular/bazel
aka ngc_wrapped, the upstream@bazel/typescript
CompilerHost
is patched to treat .ngfactory files the same as theiroriginal source file, with respect to tsickle processing.
It is currently not possible to test this change as we don't have any test
that inspects tsickle output with bazel. It will be extensively tested in
g3.
in
ngc
, Angular's own implementation is adjusted to allow for theprocessing of shims when compiling with Ivy. This enables a unit test to
be written to validate the correct behavior of tsickle when given a host
that's appropriately configured to process factory shims.
For ngtsc-as-a-plugin, a similar fix will need to be submitted upstream in
tsc_wrapped.