Fix cross imports and avoid code duplication/singleton dependency tracking bugs by devversion · Pull Request #51500 · angular/angular

devversion · 2023-08-25T12:47:58Z

See individual commits. This will allow us to remove the unreliable lint rule in the components repo as well.
https://github.com/angular/components/blob/5d65f1dc4a719fd673b4d77f2f5bdba61d7ac523/tools/tslint-rules/noCrossEntryPointRelativeImportsRule.ts

Also see: #51495

JoostK

Very nice! 💯

JoostK · 2023-08-25T13:25:34Z

packages/core/rxjs-interop/src/to_signal.ts

This may also fix a bug I presume, where untracked had its own private active consumer.

Yes, good point. Do we know what issues this could cause? I assume, it might break some of the consumer notifications in e.g. change detection? (if the interop is used?)

untracked appears to be only used in an ngDevMode guarded condition, so production code won't be affected.

In development this could result in not actually untracking, resulting in a dependency where none may have been intended. It may also affect ReactiveNode.producerUpdatesAllowed, but I don't think that it would make a difference.

Thanks for capturing this here @JoostK

pmvald

Thanks! Now #51293 passes the aio local test!

packages/bazel/src/ng_package/cross_entry_points_imports.ts

pmvald · 2023-08-25T19:56:49Z

One of the culprits for this type of mistakes is IDE auto import, which sometimes add the import in the wrong format. Folks may not even notice it at that time ...

devversion · 2023-08-25T20:51:09Z

Yes, exactly. I think this is the reason why all of these imports exist that way

Introduces a check into `ng_package` that will ensure that there are no cross entry-point or cross-package relative imports that would end up contributing to duplicate code. Not only would duplicate code result in size increases, but also it could cause subtle hard-to-debug bugs, especially when cross imports rely on e.g. singletons. Like for example the deps tracker that is used in angular/core but also in angular/core/testing.

…ng, rxjs-interop) Fixes that there was code duplication between the primary entry-point, the testing entry-point and the rxjs-interop entry-point. This code duplication resulted in additional code size (really neglibible here because rxjs-interop did not duplicate large parts of core, and `testing` is not used in production). On the other hand though, the duplication resulted in a subtle JIT dependency tracking issue due to the `depsTracker` no longer being a singleton. This caused test failures as in: angular#51415.

We were collecting all ES2022 files from entry-points (including transitive files). Those are later on combined and filtered so that we know which files to copy over to the package. There was no deduping here. This did not have an effect, but could be a source of slowness in `ng_package` and also breaks validation checks which could show same errors multiple times for the same file.

The animations packages were duplicating a little bit of code due to relative imports between entry-points. This caused bundlers to inline shared functions twice in both FESM outputs.

The common packages were duplicating a little bit of code due to relative imports between entry-points. This caused bundlers to inline shared functions twice in both FESM outputs.i

The upgrade package duplicaes some of code due to relative imports between entry-points. This caused bundlers to inline shared functions twice in both FESM outputs. This is an acceptable limitation and we are not changing this because the primary entry-point is not synced into G3. It's non-trivial to remove these cross relative imports right now because the primary entry-point is not even built in G3 so instead we just ignore the relative imports using a re-export file. Note: To simplify this change, we continue using namespace exports as exporting individual named exports for all these possible usages is rather cumbersome and also we had existing namespace imports for e.g. `angular1.ts`. The code of upgrade is rarely edited these days

The localize package intentionally duplicates some logic from the compiler to avoid adding a dependency. This is now an error in the packaging rule to prevent common pitfalls/code duplication. Here it's an explicit decision though so we mark it as such and ask for the check to be ignored for the particular import.

devversion · 2023-08-28T10:44:12Z

Caretaker note: As part of syncing, please make a manual change to include upgrade/static/common.ts in the BUILD file. See: http://cl/560068028.

jelbourn

LGTM

Reviewed-for: global-approvers

packages/bazel/src/ng_package/cross_entry_points_imports.ts

The upgrade package duplicaes some of code due to relative imports between entry-points. This caused bundlers to inline shared functions twice in both FESM outputs. This is an acceptable limitation and we are not changing this because the primary entry-point is not synced into G3. It's non-trivial to remove these cross relative imports right now because the primary entry-point is not even built in G3 so instead we just ignore the relative imports using a re-export file. Note: To simplify this change, we continue using namespace exports as exporting individual named exports for all these possible usages is rather cumbersome and also we had existing namespace imports for e.g. `angular1.ts`. The code of upgrade is rarely edited these days PR Close #51500

…ge (#51500) The localize package intentionally duplicates some logic from the compiler to avoid adding a dependency. This is now an error in the packaging rule to prevent common pitfalls/code duplication. Here it's an explicit decision though so we mark it as such and ask for the check to be ignored for the particular import. PR Close #51500

…ge (angular#51500) The localize package intentionally duplicates some logic from the compiler to avoid adding a dependency. This is now an error in the packaging rule to prevent common pitfalls/code duplication. Here it's an explicit decision though so we mark it as such and ask for the check to be ignored for the particular import. PR Close angular#51500

…r package (angular#51500)

…ge (#51500) (#51558) The localize package intentionally duplicates some logic from the compiler to avoid adding a dependency. This is now an error in the packaging rule to prevent common pitfalls/code duplication. Here it's an explicit decision though so we mark it as such and ask for the check to be ignored for the particular import. PR Close #51500 PR Close #51558

Introduces a check into `ng_package` that will ensure that there are no cross entry-point or cross-package relative imports that would end up contributing to duplicate code. Not only would duplicate code result in size increases, but also it could cause subtle hard-to-debug bugs, especially when cross imports rely on e.g. singletons. Like for example the deps tracker that is used in angular/core but also in angular/core/testing. PR Close angular#51500

…ng, rxjs-interop) (angular#51500) Fixes that there was code duplication between the primary entry-point, the testing entry-point and the rxjs-interop entry-point. This code duplication resulted in additional code size (really neglibible here because rxjs-interop did not duplicate large parts of core, and `testing` is not used in production). On the other hand though, the duplication resulted in a subtle JIT dependency tracking issue due to the `depsTracker` no longer being a singleton. This caused test failures as in: angular#51415. PR Close angular#51500

We were collecting all ES2022 files from entry-points (including transitive files). Those are later on combined and filtered so that we know which files to copy over to the package. There was no deduping here. This did not have an effect, but could be a source of slowness in `ng_package` and also breaks validation checks which could show same errors multiple times for the same file. PR Close angular#51500

…r#51500) The animations packages were duplicating a little bit of code due to relative imports between entry-points. This caused bundlers to inline shared functions twice in both FESM outputs. PR Close angular#51500

) The common packages were duplicating a little bit of code due to relative imports between entry-points. This caused bundlers to inline shared functions twice in both FESM outputs.i PR Close angular#51500

…51500) The upgrade package duplicaes some of code due to relative imports between entry-points. This caused bundlers to inline shared functions twice in both FESM outputs. This is an acceptable limitation and we are not changing this because the primary entry-point is not synced into G3. It's non-trivial to remove these cross relative imports right now because the primary entry-point is not even built in G3 so instead we just ignore the relative imports using a re-export file. Note: To simplify this change, we continue using namespace exports as exporting individual named exports for all these possible usages is rather cumbersome and also we had existing namespace imports for e.g. `angular1.ts`. The code of upgrade is rarely edited these days PR Close angular#51500

…ge (angular#51500) The localize package intentionally duplicates some logic from the compiler to avoid adding a dependency. This is now an error in the packaging rule to prevent common pitfalls/code duplication. Here it's an explicit decision though so we mark it as such and ask for the check to be ignored for the particular import. PR Close angular#51500

angular-automatic-lock-bot · 2023-09-29T00:08:45Z

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