Skip to content

Refactors Zones to reduce top-level side effects#53443

Closed
dgp1130 wants to merge 34 commits intoangular:mainfrom
dgp1130:zone-imports
Closed

Refactors Zones to reduce top-level side effects#53443
dgp1130 wants to merge 34 commits intoangular:mainfrom
dgp1130:zone-imports

Conversation

@dgp1130
Copy link
Copy Markdown
Contributor

@dgp1130 dgp1130 commented Dec 8, 2023

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:

// some-file.ts

// Immediately runs patch.
Zone.__load_patch('...', () => {
  // ...
});

Into:

// some-file.ts

import {ZoneType} from 'zone.js';

// Exports a function which will run the patch.
export function patchStuff(Zone: ZoneType): void {
  Zone.__load_patch('...', () => {
    // ...
  });
}

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 the patch* functions at top-level scope, so consumers of zone.js shouldn'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

@jessicajaniuk jessicajaniuk added the area: zones Issues related to zone.js label Dec 8, 2023
@ngbot ngbot Bot added this to the Backlog milestone Dec 8, 2023
@dgp1130 dgp1130 force-pushed the zone-imports branch 3 times, most recently from 0d8aff2 to 96b82c1 Compare December 9, 2023 02:58
@dgp1130 dgp1130 force-pushed the zone-imports branch 3 times, most recently from 69e9d20 to 6b6f26a Compare January 12, 2024 23:17
@dgp1130 dgp1130 changed the title Refactors Zones to use real imports Refactors Zones to reduce top-level side effects Jan 16, 2024
@dgp1130 dgp1130 force-pushed the zone-imports branch 3 times, most recently from 87ad4c1 to 9a6c5b9 Compare January 17, 2024 02:00
@dgp1130 dgp1130 force-pushed the zone-imports branch 7 times, most recently from 945c209 to d56e0e1 Compare January 31, 2024 22:17
@dgp1130 dgp1130 force-pushed the zone-imports branch 11 times, most recently from 95d46ce to 9d04f9b Compare February 2, 2024 05:00
dgp1130 added 7 commits March 14, 2024 14:04
…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.
@alxhub alxhub added the target: major This PR is targeted for the next major release label Mar 16, 2024
@alxhub
Copy link
Copy Markdown
Member

alxhub commented Mar 16, 2024

This PR was merged into the repository by commit 5328be6.

@alxhub alxhub closed this in a760470 Mar 16, 2024
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
#53443)

This allows the types to be directly imported when used.

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
This removes a name collision on `Zone` with the interface. Otherwise putting the `interface Zone {}` and `class Zone {}` in the same scope when the IIFE is removed will cause a conflict.

PR Close #53443
alxhub pushed a commit that referenced this pull request Mar 16, 2024
…oking it as an IIFE (#53443)

This removes the top-level side effects from this file. Indenting is left as-is to minimize changes.

PR Close #53443
alxhub pushed a commit that referenced this pull request Mar 16, 2024
All its content has been copied to `zone-impl.ts`, so this file will become a wrapper for those symbols.

PR Close #53443
alxhub pushed a commit that referenced this pull request Mar 16, 2024
…zone.ts` (#53443)

Global state is to be managed in `zone.ts` rather than `zone-impl.ts`.

PR Close #53443
alxhub pushed a commit that referenced this pull request Mar 16, 2024
…3443)

Now that we have real ES modules, there's no need for the prefix convention.

PR Close #53443
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
)

`zone.ts` contains the global variable definition and is the appropriate place for an error side effect when Zone is initialized twice.

PR Close #53443
alxhub pushed a commit that referenced this pull request Mar 16, 2024
…zone.ts` (#53443)

This helps consolidate globals into a single declaration in a single file.

PR Close #53443
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
… function (#53443)

This drop the top-level side effects in `zone.ts` and allows the initialization to be reused across the various entry points in the package.

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
@angular-automatic-lock-bot
Copy link
Copy Markdown

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: zones Issues related to zone.js merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants