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

feat: support emitting asset with custom asset info #3837

Closed
wants to merge 38 commits into from

Conversation

suica
Copy link
Contributor

@suica suica commented Jul 25, 2023

WIP

draft for #3701

Summary

This PR allows users to emit asset with custom asset info.

TODOs (for personal tracking of progress):

  • Change the signature of emitAsset in binding file.
    • Modify emit_asset signature, at crates/node_binding/src/js_values/compilation.rs.
  • Handle the extra fields. In internal emit_asset, at crates/rspack_core/src/compiler/compilation.rs.
    • Make a demo which embeds all_map into the AssetInfo&JsAssetInfo type, to show that we can accept info that users give.
    • Modify the demo, and implement a rather complete version. Either modify type JsAssetInfo or just use type AssetInfoMap instead.
  • Add the missing AssetInfoMap type in generation of binding file.
    • Ref: export type AssetInfo = Partial<JsAssetInfo> & Record<string, any>; at packages/rspack/src/compilation.ts.
    • Ref: type AssetInfo = KnownAssetInfo & Record<string, any>; at node_modules/webpack/types.d.ts.
    • Ref: https://napi.rs/docs/concepts/types-overwrite.en
  • Do the similar to JsStatsAsset. Make sure we can emit the custom info object into the stats.
  • Remove JsAssetInfo.
  • Add support for other Webpack APIs that will modify asset info, e.g. updateAsset at crates/node_binding/src/js_values/compilation.rs.
  • Test the code. Add more test cases.
  • Clean the code.

Notes

  1. Type has an improper name.
	compilation.hooks.processAssets.tap("Plugin", assets => {

type of assets is suspicious: export type Assets = Record<string, Source>;. Why source?

  1. Clear dist before test. Otherwise old main.js will lead to a failed case.

  2. Cannot compile

Already reported this to Brooklyn and will be fixed.

#[napi(object)]
pub struct Foo {
  pub a: String,
}

#[napi]
pub fn can_compile(b: Either<std::collections::HashMap<String, serde_json::Value>, Foo>) {}

#[napi]
pub fn can_compile2(b: serde_json::Map<String, serde_json::Value>) {}

#[napi]
pub fn cannot_compile(b: Either<serde_json::Map<String, serde_json::Value>, Foo>) {}

Test Plan

TBA

@suica suica changed the title feat: support custom asset info feat: support emitting asset with custom asset info Jul 25, 2023
@h-a-n-a h-a-n-a self-assigned this Jul 26, 2023
@suica
Copy link
Contributor Author

suica commented Jul 26, 2023

在传入提供的info对象的时候,这个info对象会出现在对应的asset的stats info中。
当前rspack的行为是,asset stats info必定会包含这两个字段,但是按照webpack的行为,是不会主动包含development/hmr字段的,除非用户在emit的时候提供了info对象。

我的问题:

  1. 在支持自定义info对象的时候,是否仍然需要额外加上这两个字段,还是与webpack严格保持一致?
  2. 如果选择加上这两个字段,那么要不要干脆直接把整个asset info原原本本地扔出去、而不是只抠出这两个字段。

@h-a-n-a Thanks!

@stale
Copy link

stale bot commented Sep 25, 2023

This pull request has been automatically marked as stale because it has not had recent activity. If this pull request is still relevant, please leave any comment (for example, "bump").

@stale stale bot added the stale label Sep 25, 2023
@stale stale bot removed the stale label Oct 16, 2023
@JoseBuendiaDigio
Copy link

Any news about this?

@suica
Copy link
Contributor Author

suica commented Nov 24, 2023

Any news about this?

Sorry, I think I am going to finish the left two TODOs very soon :)

Copy link

stale bot commented Jan 24, 2024

This pull request has been automatically marked as stale because it has not had recent activity. If this pull request is still relevant, please leave any comment (for example, "bump").

@stale stale bot added the stale label Jan 24, 2024
@hardfist
Copy link
Contributor

@suica are you interested in working on this?

@h-a-n-a
Copy link
Contributor

h-a-n-a commented Jun 12, 2024

Sorry for not being able to follow up on this one. It's moved to the new PR #6721. Thank you for the contribution!

@h-a-n-a h-a-n-a closed this Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants