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(@angular-devkit/build-angular): transform remapped sourcemap into a plain object #22220

Merged
merged 1 commit into from
Nov 23, 2021

Conversation

divdavem
Copy link
Contributor

@divdavem divdavem commented Nov 23, 2021

Hello,

I am opening this PR to fix one of the problems we noticed when upgrading to Angular 13 in ng-bootstrap (cf ng-bootstrap/ng-bootstrap#4178)

We are using a custom webpack configuration with @angular-builders/custom-webpack:browser to add coverage-istanbul-loader but it fails with the don't know how to turn this value into a node error.

This similar to #21967 which was previously fixed in commit da1733c but the fix was lost as part of commit 0c44ab3.
Cf also istanbuljs/istanbuljs#646

remapping returns a SourceMap object and not a plain object. This causes Babel to fail with don't know how to turn this value into a node when invoked from istanbul-lib-instrument as Babel checks if the value is a plain object.

See: https://github.com/babel/babel/blob/780aa48d2a34dc55f556843074b6aed45e7eabeb/packages/babel-types/src/converters/valueToNode.ts#L115-L130

… a plain object

`remapping` returns a SourceMap object and not a plain object. This causes
Babel to fail with `don't know how to turn this value into a node` when
invoked from `istanbul-lib-instrument` as Babel checks if the value is a
plain object.

See: https://github.com/babel/babel/blob/780aa48d2a34dc55f556843074b6aed45e7eabeb/packages/babel-types/src/converters/valueToNode.ts#L115-L130

This was previously fixed in commit da1733c
but the fix was lost as part of commit 0c44ab3.
@google-cla google-cla bot added the cla: yes label Nov 23, 2021
@divdavem divdavem marked this pull request as draft November 23, 2021 16:04
@divdavem divdavem marked this pull request as ready for review November 23, 2021 16:22
@alan-agius4
Copy link
Collaborator

alan-agius4 commented Nov 23, 2021

This was intentionally removed.

Now in the CLI we only do a single Babel pass. Previously, this workaround was needed because of the usage of coverage-istanbul-loader which caused an additional Babel pass.

The usage of this webpack loader has been removed in v13 and instead we use the official Istanbul Babel plugin https://github.com/istanbuljs/babel-plugin-istanbul which also has better source map fidelity and caches instrumented code which causes faster testing times.

Can you share a reproduction on how you are experiencing the error without the mentioned loader?

@divdavem
Copy link
Contributor Author

@alan-agius4 Thank you for your answer. I actually updated the description of the issue which should answer your question:

We are using a custom webpack configuration with @angular-builders/custom-webpack:browser to add coverage-istanbul-loader but it fails with the don't know how to turn this value into a node error.

cf https://github.com/ng-bootstrap/ng-bootstrap/blob/master/e2e-app/coverage.webpack.js
Note that it is currently temporarily not used anymore (because of this error) cf this commit: ng-bootstrap/ng-bootstrap@9d6e62b

@divdavem
Copy link
Contributor Author

Note that it would be good actually to be able to enable instrumentation from an option in angular.json and then benefit from the improvements you did in Angular 13 (single pass transformation and improved source map fidelity) for our e2e test coverage use case (thus avoiding the use of our custom webpack configuration).

@divdavem
Copy link
Contributor Author

So the issue can be reproduced with:

git clone https://github.com/ng-bootstrap/ng-bootstrap.git
cd ng-bootstrap
# the current head commit in the master branch should already be 37c84a6241dc2465c47dc2b357c6f07313569b99 anyway
git checkout 37c84a6241dc2465c47dc2b357c6f07313569b99
git revert 9d6e62b548a7bce799f604188a4502163a280e50
yarn
ng serve e2e-app -c playwright

@alan-agius4 alan-agius4 added target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker labels Nov 23, 2021
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

@alan-agius4 alan-agius4 merged commit 5805c78 into angular:master Nov 23, 2021
@divdavem
Copy link
Contributor Author

@alan-agius4 Thank you for merging my PR!

@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 Dec 25, 2021
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 target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants