Skip to content

fix(bazel): fix incorrect rollup plugin method signature#41101

Closed
jbedard wants to merge 1 commit intoangular:masterfrom
jbedard:bazel-resolve-signature
Closed

fix(bazel): fix incorrect rollup plugin method signature#41101
jbedard wants to merge 1 commit intoangular:masterfrom
jbedard:bazel-resolve-signature

Conversation

@jbedard
Copy link
Contributor

@jbedard jbedard commented Mar 6, 2021

Update the resolveBazel method signature to align with the rollup plugin
resolveId API: https://rollupjs.org/guide/en/#resolveid

PR Type

Bugfix

What is the current behavior?

The incorrect plugin signature causes a crash in rollup >=2.29.0 after the extra argument was added: rollup/rollup@1ad8289#diff-2651238122dc82e0e84b4e3cb1499b8cbd4c01a21a36c9c2fae5812e95cd8ec8R233

What is the new behavior?

It doesn't crash

Does this PR introduce a breaking change?

No

Other information

I hope minor bug fixes are still accepted for @angular/bazel?

#35149 suggests updating the @angular/bazel peer deps to more modern versions of rollup + plugins which I'd also like to do if such changes are still accepted? However this PR is a minor fix without any breaking changes so I thought I'd start with this...

Update the resolveBazel method signature to align with the rollup plugin
resolveId API: https://rollupjs.org/guide/en/#resolveid
Copy link
Contributor

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Seems like Angular ought to use ng-packagr to package itself, as that's the supported solution in angular-cli.

That would allow deprecation and removal of the @angular/bazel package?

If ng-packagr doesn't do all the right things, seems like it should be fixed there rather than maintain this competing thing I wrote 😞

@jbedard
Copy link
Contributor Author

jbedard commented Mar 7, 2021

I'm not sure how compatible ng-packagr would be with things like ts_library though? Otherwise yeah, I'd rather just use that...

@alexeagle
Copy link
Contributor

There's some work for sure - I imagine the layering would have to be changed somewhat if ng_package rule is doing more than it should.

@jbedard
Copy link
Contributor Author

jbedard commented Mar 7, 2021

@petebacondarwin note that this doesn't actually fix #35149. That is to upgrade the rollup peerdep where this fixes a crash once you do upgrade rollup. So this will be required if #35149 is done, but I'd hope this one can be merged first either way...

Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

@josephperrott josephperrott added action: merge The PR is ready for merge by the caretaker area: bazel Issues related to the published `@angular/bazel` build rules target: patch This PR is targeted for the next patch release labels Mar 8, 2021
@ngbot ngbot bot added this to the Backlog milestone Mar 8, 2021
AndrewKushnir pushed a commit that referenced this pull request Mar 8, 2021
Update the resolveBazel method signature to align with the rollup plugin
resolveId API: https://rollupjs.org/guide/en/#resolveid

PR Close #41101
This was referenced Mar 15, 2021
@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 Apr 8, 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 area: bazel Issues related to the published `@angular/bazel` build rules cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@angular/bazel peer-depends on deprecated rollup packages

3 participants