Refactors Zones to reduce top-level side effects#53443
Closed
dgp1130 wants to merge 34 commits intoangular:mainfrom
Closed
Refactors Zones to reduce top-level side effects#53443dgp1130 wants to merge 34 commits intoangular:mainfrom
dgp1130 wants to merge 34 commits intoangular:mainfrom
Conversation
0d8aff2 to
96b82c1
Compare
69e9d20 to
6b6f26a
Compare
87ad4c1 to
9a6c5b9
Compare
945c209 to
d56e0e1
Compare
95d46ce to
9d04f9b
Compare
…calling patches appropriately This executes the patches in the top-level scope of the Bluebird test.
The lazy policy tests call the same error policy entry point, so this fixes both at the same time.
Not sure why this becomes a problem now. These errors appear to be preexisting. Most likely the file wasn't loaded previously and now it is.
…`electron`) as external
The `rxjs|electron` regex was causing `import { /* ... */ } from './electron';` to be treated as external and not bundled, which is an issue for the Electron entry point which just happens to be named `electron.ts`. Now only the actual `electron` package should be treated as external, while internal files of any name should not.
I actually tried updating to `^rxjs` as well, however this appears to break some tests and isn't necessary to fix the failing Electron test, so I'm leaving it alone.
This was a bit complicated, but the typings test (`packages/zone.js/test/typings/...`) was failing due to an unresolved import on `./zone-impl`. The main cause is that `//packages/zone.js:zone_js_d_ts` was generating the output `zone.d.ts` file by _concatenating_ `zone.d.ts` with `zone.api.extensions.d.ts` and `zone.configurations.api.d.ts`. Now that `zone.d.ts` imports `zone-impl.d.ts`, concatenation is no longer a viable means of bundling this content. To fix this, I created a new `packages/zone.js/zone.ts` entry point and imported the underlying `zone.ts` file as well as the two extensions. I added `extract_types` to pull the `*.d.ts` files out of the target (because all the JS is bundled separately) and used those files in the final NPM package. This is sufficient to pass the typings test and should be equivalent to what exists today.
I initially tried switching to use public entry points under `zone.js/plugins/*`, however this file is both manually compiled for Saucelabs and also built with Bazel for a number of tests. The Bazel integration doesn't work well with depending on real NPM packages, so importing `zone.js/plugins/*` in that context doesn't really work. Instead we need to depend on the internals and manually call the `patch*` functions.
For some reason CI started complaining about lack of formatting here.
Member
|
This PR was merged into the repository by commit 5328be6. |
alxhub
pushed a commit
that referenced
this pull request
Mar 16, 2024
This drops global declarations from this file (as they will be limited to `zone.ts`. It also deletes the `/** @internal */` annotation on `AmbientZone` which is no longer necessary since it isn't in the `declare global` and deletes the global Zone declaration which is also unnecessary. PR Close #53443
alxhub
pushed a commit
that referenced
this pull request
Mar 16, 2024
The IIFE will be removed shortly to support exporting symbols inside it. PR Close #53443
alxhub
pushed a commit
that referenced
this pull request
Mar 16, 2024
alxhub
pushed a commit
that referenced
this pull request
Mar 16, 2024
alxhub
pushed a commit
that referenced
this pull request
Mar 16, 2024
These are all the symbols originally under `declare global {}` with the same names. Unfortunatley TypeScript treats a `declare global {}` scope as the parent scope, so we need to import with a different name than the global, meaning everything gets an `_` prefix.
PR Close #53443
alxhub
pushed a commit
that referenced
this pull request
Mar 16, 2024
alxhub
pushed a commit
that referenced
this pull request
Mar 16, 2024
…ns (#53443) This removes top-level side effects from each of these files and drops the dependency on global `Zone`, instead allowing it to be provided to each patch as a parameter. Most of these are pure mechanical transformations. A couple notable files which were somewhat unique: * `async-test.ts`, `fake-async-test.ts`, and `wtf.ts` had unique IIFE usage and patch `Zone` itself. This removes the IIFE and exports the function instead. * `jest.ts` and `jasmine.ts` have a unique `jest` global usage which needs to be declared. PR Close #53443
alxhub
pushed a commit
that referenced
this pull request
Mar 16, 2024
… function. (#53443) This particular is slightly different from the others because it does not have a hard dependency on `Zone`. It doesn't actually use `Zone` directly during the patch because the patch just sets a `legacyPatch` global. To maintain consistency, this continues to use global `Zone` and does _not_ accept `Zone` as a parameter. While it's using a global in an inconvenient way, this isn't a problem right now because it doesn't cause or require a dependency on a top-level side effect. PR Close #53443
alxhub
pushed a commit
that referenced
this pull request
Mar 16, 2024
…3443) Since each patch no longer contains top-level side effects, each bundled entry point needs to import and call its associated patch. For the most part this just means that each entry point imports the associated patch and invokes it at the top-level scope. Note that many of these entry points did not actually have a dependency on `Zone` and had no guarantee that it was loaded prior to execution. To maintain consistency, the missing dependencies on `Zone` are left as-is. They will use the global instance of `Zone` and if users fail to load it prior to importing a specific patch, then the patch will fail just as it did previously. PR Close #53443
alxhub
pushed a commit
that referenced
this pull request
Mar 16, 2024
This removes a dependency on a top-level side effect of global Zone initialization. PR Close #53443
alxhub
pushed a commit
that referenced
this pull request
Mar 16, 2024
While `ProxyZoneSpec` was implicitly global previously, it does not need to be exposed in a `declare global {}` block. This is because classic scripts in TypeScript are only implicitly global within the same compilation. `ProxyZoneSpec` was not exposed in the existing `.d.ts` files shipped with the `zone.js` package. Within google3, this is also a separate compilation and was not accessible. As a result, `ProxyZoneSpec` was always private and we do not need a global to maintain compatibility.
PR Close #53443
alxhub
pushed a commit
that referenced
this pull request
Mar 16, 2024
…3443) While `TaskTrackingZoneSpec` was implicitly global previously, it does not need to be exposed in a `declare global {}` block. This is because classic scripts in TypeScript are only implicitly global within the same compilation. `TaskTrackingZoneSpec` was not exposed in the existing `.d.ts` files shipped with the `zone.js` package. Within google3, this is also a separate compilation and was not accessible. As a result, `TaskTrackingZoneSpec` was always private and we do not need a global to maintain compatibility. PR Close #53443
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The goal here is to remove top-level side effects of most Zone patches and convert them into functions which can be called at any time. Basically the key outcome of this PR is to change:
Into:
This makes execution order much more intuitive, since the timing of specific statements depends on when the patch functions are invoked rather than where the bundler happens to place the file based on import structure. It also better supports individual patches being tree shaken.
For now, this change is attempting (unsuccessfully) to be as small and non-breaking as possible for the change it's trying to make. None of the new
patch*functions are exported and all the existing entry points still invoke thepatch*functions at top-level scope, so consumers ofzone.jsshouldn't notice the change at all. There may be some small re-ordering between files, but hopefully nothing breaking. There are still some top-level side effects, but they are mostly limited to test files (ex.wtf_mock.ts), so not a major concern. There are definitely a lot of potential cleanup and modernization tasks available, but I'll try to avoid increasing the scope of this PR any more than it already is.This change is quite involved and touches a lot of pieces of Zone.js infrastructure. Doing everything at once would lead to unreadable diffs, so instead I made a bunch of small, mechanical changes in individual commits to hopefully be more clear about what I'm trying to do at each step. As a result the PR isn't quite as bad as the line changed count would lead you to believe. I highly recommend reviewing this one commit at a time.
I did my best to keep runtime behavior, file ordering, statement execution timing, and global types unchanged in order to reduce risk in this change. Let me know if you see anything which could be an observable change to consumers.
/cc @JiaLiPassion @devversion @atscott @clydin