Migrate first jasmine_node_test to new RJS variant
#29349
Conversation
angular-robot
bot
added
the
area: build & ci
| /[email protected]: | ||
| resolution: {integrity: sha512-l3BikUxvPOcn5E74dZiq5BGsTb5yEwhaTSzccU6t4sDOH8NWJCstKO5QT2CvtFoK6F0saL7p9xHAqHOlCPJygA==} | ||
| engines: {node: '>= 8'} | ||
| dev: true |
There was a problem hiding this comment.
These dependencies lost their dev status because packages/angular_devkit/core requires them as non-dev. Perfectly fine!
but this brings up a good future opportunity in ensuring the root package.json matches the dependencies in the "shipped package.json". cc. @clydin. I assume this would work if we could use pnpm for publishing as it would automatically e.g. replace workspace:XXX with the proper versions.
| ":node_modules/@angular-devkit/core", | ||
| "//:node_modules/@types/node", | ||
| "//:node_modules/rxjs", | ||
| "//packages/angular_devkit/core:core_rjs", |
There was a problem hiding this comment.
The new npm-archive imports nicely prevent cross-relative imports (like we decided in MUC!). It also fixes/improves some .d.ts output generation where TS currently emits e.g. import('../core')
devversion
force-pushed
the
first_jasmine_test
branch
from
a59df90 to
decb4f4
Compare
This is necessary as `rules_js` requires this "common name" when dealing with Yarn workspaces, linking first party dependencies automatically. In the future, we may be able to send a PR to `rules_js` to support a custom name somehow.
devversion
force-pushed
the
first_jasmine_test
branch
from
decb4f4 to
cbed807
Compare
devversion
added
action: review
This is necessary as with the new compilation, leveraging pnpm-linked first party dependencies, there are cases **during migration** where multiple locations of the same package exist. e.g. - `@angular_devkit/architect/node_modules/@angular-devkit/core` (pnpm package output) - `@angular_devkit/core` (plain js sources) This is fine, and there is no risk of wrong types, and this should only occur during migration anyway, but it causes a few type issues as TypeScript is unable to emit proper types for inference. This is okay, as we'd likely even want to make use of `isolatedDeclarations` at some point, but we just add explicit types right now. (also making `Builder` type more type safe, checking assignability properly).
…les_js` Instead of using `rules_nodejs` interop and the linker at runtime, the test target is migrated to the new `jasmine_test` rule, leveraging the pnpm workspace for first-party dependencies. Note that in the future we may be able to rename the local node modules to something more clear. e.g. `:local_modules/@angular-devkit/core`, but for now this is not possible :( This commit shows how similar packages can be migrated in the future.
devversion
force-pushed
the
first_jasmine_test
branch
from
cbed807 to
c0ad5f5
Compare
| } | ||
|
|
||
| export default createBuilder(buildApplication); | ||
| const builder: Builder<ApplicationBuilderOptions & json.JsonObject> = |
There was a problem hiding this comment.
| const builder: Builder<ApplicationBuilderOptions & json.JsonObject> = | |
| const builder: Builder<ApplicationBuilderOptions & import('@angular-devkit/core').json.JsonObject> = |
I'd like to minimize the core package usage as much as possible here. I think this is as far as possible right now. Assuming it works with the other changes.
There was a problem hiding this comment.
@devversion maybe we should switch to Type instead of Interface which would avoid the & json.JsonObject.
There was a problem hiding this comment.
It seems to be used throughout the repository, within the same builders. Are we good keeping as is and just leaving this to a general cleanup? (seems more work, even with Alan's idea; which seems to work; I'm not sure of the consequences and there would still be quite some cleanup work then)
There was a problem hiding this comment.
I'm fine with some TODOs here to cleanup later in the migration process.
There was a problem hiding this comment.
Discussed offline. TODO not necessary, but we'll track this in a separate issue as cleanup.
devversion
added
action: merge
devversion
added
target: minor
|
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. |
3 participants
See individual commits (review per-commit ideally)