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.

Sign up for GitHub

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

Jump to bottom

feat: support js compilation.addInclude API #8483

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

feat: support js compilation.addInclude API #8483

wants to merge 4 commits into from
+56 −19

Conversation

GiveMe-A-Name
Copy link
Member

Summary

Add compilation.addInclude API

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

@github-actions github-actions bot added the release: feature release: feature related release(mr only) label Nov 20, 2024
@GiveMe-A-Name GiveMe-A-Name mentioned this pull request Nov 20, 2024
Open
@netlify Netlify
Copy link

netlify bot commented Nov 20, 2024 •
edited
Loading

✅ Deploy Preview for rspack canceled.

Name Link
🔨 Latest commit ebc3318
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/673d95ac4f30a7000834bcd8

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Nov 20, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #8483 will not alter performance

Comparing feat/add-compilation-addInclude (ebc3318) with main (687a891)

Summary

✅ 1 untouched benchmarks

SyMind
SyMind reviewed Nov 20, 2024
View reviewed changes
crates/rspack_binding_values/src/compilation/mod.rs Outdated
#[napi(ts_args_type = "dependency: JsDependency, options: JsEntryOptions")]
pub fn add_include(
&mut self,
dependency: JsDependencyWrapper,
Copy link
Member

@SyMind SyMind Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Use &JsDependency instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it

Copy link
Member Author

Choose a reason for hiding this comment

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

Done~

crates/rspack_binding_values/src/dependency.rs Outdated Show resolved Hide resolved
@SyMind
Copy link
Member

SyMind commented Nov 20, 2024

I'd like to know what this JavaScript API is used for in RSC?

@GiveMe-A-Name
Copy link
Member Author

I'd like to know what this JavaScript API is used for in RSC?

You can see this plugin that implement in webpack
https://github.com/web-infra-dev/modern.js/blob/feat-next-wym/packages/cli/uni-builder/src/shared/rsc/plugins/rsc-server-plugin.ts#L101-L128

@SyMind
Copy link
Member

SyMind commented Nov 20, 2024

Firstly, I haven't found the way in which addInclude is used in Next.js and Modern.js.

Secondly, I think that addInclude is supposed to receive a Dependency that is manually constructed by developers on the JavaScript side. However, there isn't a relevant JavaScript API available for this at present.

@GiveMe-A-Name GiveMe-A-Name marked this pull request as draft November 20, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahabhgk ahabhgk Awaiting requested review from ahabhgk

@SyMind SyMind Awaiting requested review from SyMind

At least 1 approving review is required to merge this pull request.

Assignees

@GiveMe-A-Name GiveMe-A-Name

Labels
release: feature release: feature related release(mr only)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@GiveMe-A-Name @SyMind