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

fix(migrations,ng-add): turn on encapsulation for devkit/schematics deps #13712

Merged
merged 6 commits into from
Dec 5, 2023

Conversation

damyanpetev
Copy link
Member

@damyanpetev damyanpetev commented Nov 27, 2023

Related to #13668

As suggested by the Angular Team - using "encapsulation": true for the migration/schematics collections. This allows imports (well, require-s really in the node context), to pass through the Angular CLI-s context - i.e. resolve that package's deps. While useful if the tree messes up and our imports won't fail, it should in theory also more closely guarantee the version used, assuming the ng CLI is updated first.

The related code that handles the encapsulation option mostly lives here: https://github.com/angular/angular-cli/blob/4da732ddf686198ae598698dc4c74515d46624f0/packages/angular/cli/src/command-builder/utilities/schematic-engine-host.ts

This is based on Node's module.createRequire to create a context-bound require from the Angular CLI and override in consumed schematics using the VM context API to run said script with the overrides via vm.runInNewContext.
Note that this restricts the global API surface schematics/migrations can access as from what I can tell the new context is final in the sense that it doesn't backfill global API if not provided and so the list is fairly short:
https://github.com/angular/angular-cli/blob/4da732ddf686198ae598698dc4c74515d46624f0/packages/angular/cli/src/command-builder/utilities/schematic-engine-host.ts#L218-L234
The basics (__dirname, __filename, console, process) are available, but we should be really careful not to step outside of those in the direct source (supporting packages should still be okay).

Edit: Apparently will also need to be strict about circular refs, since that wrap is recursive and doesn't handle circular refs like node would, leading to 'Maximum call stack size exceeded' error.
The other part we were warned about are dynamic imports, which are not supported. The one we use atm was a workaround, will investigate if possible to remove.

Update:
Okay, so for the most part it seems we're not really going too far outside of the supported API. However, the dynamic import is still needed for @angular/compiler since that's ESM and schematics are strictly CommonJS still and there's no alternative, regardless if we use the pre-compiled js or not. The warning turned out true though, see the new vm.Script notes under importModuleDynamically. So that's not supported yet at all (behind experimental flag) and I get that error if a migration uses that.
I do have a very cheeky workaround for that - if we require the specific file that only does the dynamic import as non-relative, it's a way to escape the wrap functionality and have that file run in the normal non-encapsulated context:
image
https://github.com/damyanpetev/bellum-gens/commits/encapsulation-test

Note: This will require us to use that in the source and some work for the licensed pipelines to edit it.

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

@damyanpetev
Copy link
Member Author

Okay, I've pushed the change to use the bare specifier to escape the encapsulation on that specific helper so we can still use the dynamic import and branch should be working/testable now.
Not a whole lot I can find regarding this specific use case, but the docs on https://nodejs.org/docs/latest-v18.x/api/esm.html#import-specifiers don't explicitly forbid it and without going too deep in checks the https://nodejs.org/docs/latest-v18.x/api/esm.html#resolution-algorithm does mention even selfUrl so for the time being I'm leaning towards this being valid use.

There are path maps for the build and a task in the licensed pipe to update the package name in place already. I suspect debugging locally might be impacted, yet to try.
Note this is still a bit fragile (like the rest in there really) as it relies on the current encapsulation check mechanism in the schematics engine.

@damyanpetev damyanpetev marked this pull request as ready for review November 29, 2023 12:16
@damyanpetev damyanpetev added the ❌ status: awaiting-test PRs awaiting manual verification label Nov 29, 2023
@Lipata Lipata self-assigned this Dec 4, 2023
@Lipata Lipata added 💥 status: in-test PRs currently being tested and removed ❌ status: awaiting-test PRs awaiting manual verification labels Dec 4, 2023
@Lipata
Copy link
Member

Lipata commented Dec 5, 2023

Tested with a very old version of igniteui-angular-samples browser and I've run all the existing 32 migrations with success. The 'ng add' schematic is also running with no problems. The previous tests are in addition to the test for the app in the related bug.

@damyanpetev you can create a branch to master.

@Lipata Lipata added ✅ status: verified Applies to PRs that have passed manual verification and removed 💥 status: in-test PRs currently being tested labels Dec 5, 2023
@Lipata Lipata merged commit d632dd2 into 17.0.x Dec 5, 2023
6 checks passed
@Lipata Lipata deleted the dpetev/migrations-17 branch December 5, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migrations ng add version: 17.0.x ✅ status: verified Applies to PRs that have passed manual verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants