Skip to content

feat: support skipSelf option in PluginContext.resolve#1703

Merged
underfin merged 10 commits intorolldown:mainfrom
kermanx:feat/skip_self
Aug 5, 2024
Merged

feat: support skipSelf option in PluginContext.resolve#1703
underfin merged 10 commits intorolldown:mainfrom
kermanx:feat/skip_self

Conversation

@kermanx
Copy link
Copy Markdown
Contributor

@kermanx kermanx commented Jul 24, 2024 •
edited
Loading

Support skipSelf option in PluginContext.resolve. Listed in #1554 and #819.

Todos

  • composingJsPlugins.
  • Tests failure: cannot handle skipped & unskipped resolving simultaneously.

Notes

The skipSelf option should default to true:

https://github.com/rollup/rollup/blob/28546b5821efcb72c2eb05f422d986524647a0e3/src/utils/PluginContext.ts#L78

@netlify
Copy link
Copy Markdown

netlify Bot commented Jul 24, 2024 •
edited
Loading

✅ Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit e17edd5
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/66b0add4c8e2c20008565c2e

@underfin underfin self-assigned this Jul 24, 2024
@hyf0
Copy link
Copy Markdown
Member

hyf0 commented Jul 24, 2024 •
edited
Loading

@underfin Should consider the affects to the feature composingJsPlugins. This behavior seems to break the composingJsPlugins feature if it's used.

@hyf0 hyf0 self-assigned this Aug 3, 2024
@kermanx kermanx marked this pull request as ready for review August 3, 2024 05:47
@kermanx kermanx marked this pull request as draft August 3, 2024 05:51
@hyf0

This comment was marked as outdated.

Sign in to view
@hyf0 hyf0 added the on-hold label Aug 3, 2024
@hyf0 hyf0 removed the on-hold label Aug 5, 2024
@hyf0
Copy link
Copy Markdown
Member

hyf0 commented Aug 5, 2024

@underfin At me after you finish the review, I need to apply some changes too.

@underfin
Copy link
Copy Markdown
Contributor

underfin commented Aug 5, 2024

@kermanx I refactor the code to avoid using resolve_skip: RwLock<Vec<Arc<HookResolveIdSkipped>>>, it could cause unexpected behavior because it hasn't different plugin context. I create new PluginContext to store it as same as rollup.

Tests failure: cannot handle skipped & unskipped resolving simultaneously.

@kermanx Could you point to me the test case?

@underfin
Copy link
Copy Markdown
Contributor

underfin commented Aug 5, 2024

@underfin At me after you finish the review, I need to apply some changes too.

I'm done.

hyf0
hyf0 reviewed Aug 5, 2024
View reviewed changes
Comment thread crates/rolldown_plugin/src/plugin_driver/mod.rs
hyf0
hyf0 reviewed Aug 5, 2024
View reviewed changes
Comment thread crates/rolldown_plugin/Cargo.toml
anyhow = { workspace = true }
arcstr = { workspace = true }
async-trait = { workspace = true }
oxc_index = { workspace = true }
Copy link
Copy Markdown
Member

@hyf0 hyf0 Aug 5, 2024

Choose a reason for hiding this comment

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

This is intentional. I don't want plugin build blocked by the whole oxc build.

@hyf0 hyf0 marked this pull request as ready for review August 5, 2024 10:49
hyf0
hyf0 approved these changes Aug 5, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@hyf0 hyf0 left a comment

Choose a reason for hiding this comment

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

I don't see any negative part anymore other than the skipped_resolve_calls fields.

@underfin cc Rerun the merge check after all assignee's approve the PR and the CI will get passed.

@hyf0
Copy link
Copy Markdown
Member

hyf0 commented Aug 5, 2024

Thanks for bring up this really nice PR. It really helps us a lot on pushing this feature forward.

@underfin underfin added this pull request to the merge queue Aug 5, 2024
Merged via the queue into rolldown:main with commit a18a8da Aug 5, 2024
@7086cmd 7086cmd deleted the feat/skip_self branch August 14, 2024 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hyf0 hyf0 hyf0 approved these changes

+1 more reviewer

@underfin underfin underfin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@underfin underfin

@hyf0 hyf0

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kermanx @hyf0 @underfin