Skip to content

Conversation

devversion
Copy link
Member

See individual commits

@devversion devversion added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Jan 20, 2025
@devversion devversion marked this pull request as ready for review January 20, 2025 16:08
@angular-robot angular-robot bot added area: build & ci Related the build and CI infrastructure of the project area: @angular-devkit/schematics labels Jan 20, 2025
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a small correction.

@alan-agius4 alan-agius4 added target: minor This PR is targeted for the next minor release action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed target: patch This PR is targeted for the next patch release action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 20, 2025
…ia test runner in pnpm workspaces

Currently when operating within a pnpm workspace and leveraging the
schematic test runner, there are situations where e.g.
`@schematics/angular` cannot be resolved. Consider this pnpm node
modules structure:

```
packages/
  pwa/
    node_modules/@schematics/angular --> .pnpm-store/@schematics/angular/...
    node_modules/@angular-devkit/schematics --> .pnpm-store/@angular-devkit/schematics/...
    index_spec.js // trying to call external schematic `@schematics/angular`
```

This above setup will fail because `@schematics/angular` is attempted to
be resolved from within the devkit schematics code, which doesn't have
access, or a dependency on `@schematics/angular`. We can use the
specified collection of the test runner to determine a good "resolution
lookup site", similiar to how it happens with the real `ng update`
command.
This commit changes the execution of `@angular/pwa` tests to `rules_js`
native `jasmine_test`.

This requires setting it up in the pnpm workspace for first-party linked
dependencies. Notably it turns out the peer dependency placeholder was
incorrect, so we are fixing it here and nicely avoid a problem where
pnpm would otherwise not find a local, or external suitable version.

As we originally tried to work without the fix for the peer dependency
range, there was supported added for extra substitutions. We are keeping
that logic as it will likely be useful in the future.
Migrates the `@angular-devkit/build-webpack` jasmine tests to
`rules_js`. This requires wiring up in the pnpm workspace.

Additionally `typescript` is added as it was missing as a "peer
dependency" at runtime for `@ngtools/webpack`. This is expected, and
we're already adding other peer deps before this change.
@devversion devversion added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 21, 2025
@devversion devversion merged commit 14584f0 into angular:main Jan 21, 2025
30 of 31 checks passed
@devversion devversion deleted the more-jasmien branch January 21, 2025 08:25
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 21, 2025
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: @angular-devkit/schematics area: build & ci Related the build and CI infrastructure of the project target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants