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(config): snapshot.module accept { timestamp: true} #6387

Merged
merged 2 commits into from
Apr 29, 2024

Conversation

SoonIter
Copy link
Member

Summary

close #6381

{ timestamp: true} should has the same meaning of { timestamp: true, hash: false }

We need to process this default value when we pass it to rust side.

image

Checklist

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

@github-actions github-actions bot added the release: bug fix release: bug related release(mr only) label Apr 28, 2024
Copy link

netlify bot commented Apr 28, 2024

Deploy Preview for rspack canceled.

Name Link
🔨 Latest commit e3ba82e
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/662f1fb7174c6800072d6827

@SoonIter SoonIter requested a review from ahabhgk April 28, 2024 14:19
@ahabhgk ahabhgk force-pushed the fix/snapshot-config branch from 2d58ab7 to e3ba82e Compare April 29, 2024 04:19
@ahabhgk ahabhgk enabled auto-merge (squash) April 29, 2024 04:20
@ahabhgk ahabhgk merged commit 3b313bb into web-infra-dev:main Apr 29, 2024
29 checks passed
@SoonIter
Copy link
Member Author

SoonIter commented Apr 29, 2024

@ahabhgk
Is this change right?

// rspack.config.ts
snapshot: {
    module: {
      hash: true,
    },
    resolve: {
      hash: true,
    },
}

This config should expectedly correspond to

// rspack.config.ts
snapshot: {
    module: {
      hash: true,
      timestamp: false,
    },
    resolve: {
      hash: true,
      timestamp: false,
    },
}

but after this change

    D(snapshot, "module", {});
	assertNotNill(snapshot.module);
	D(snapshot.module, "timestamp", true);
	D(snapshot.module, "hash", production);

it will be correspond to 🤔

// rspack.config.ts
snapshot: {
    module: {
      hash: true
      timestamp: true,
    },
    resolve: {
      hash: true,
      timestamp: true,
    },
}

{ hash: true } { timestamp: true } and { timestamp: true, hash: true } should be three different behaviors.
I think this is a very misleading configuration.

https://webpack.js.org/configuration/other-options/#builddependencies

image

{ hash: true } -> { timestamp: false, hash: true }
{ timestamp: true } -> { timestamp: true, hash: false }
{ timestamp: true, hash: true }

@ahabhgk
Copy link
Contributor

ahabhgk commented Apr 29, 2024

Oh I see.. Would you send another PR?

@SoonIter
Copy link
Member Author

Would you send another PR?

My pleasure, #6399

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: bug fix release: bug related release(mr only)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: setting snapshot options cause a build error of type AssertionError [ERR_ASSERTION]
2 participants