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(packages/rspack): add version check for binding #3717

Merged
merged 5 commits into from
Jul 20, 2023

Conversation

lippzhang
Copy link
Contributor

@lippzhang lippzhang commented Jul 6, 2023

Summary

Add version consistency check for core, binding and addon.
closes #3715

Test Plan

ecosystem-ci passing

@lippzhang lippzhang requested review from a team, LingyuCoder and jerrykingxyz July 6, 2023 09:00
@lippzhang lippzhang force-pushed the feature/lippzhang-bettererror branch 4 times, most recently from 634cb42 to 4f9233b Compare July 6, 2023 09:15
@h-a-n-a h-a-n-a requested a review from hardfist July 6, 2023 09:33
@lippzhang lippzhang force-pushed the feature/lippzhang-bettererror branch from 4f9233b to 2d0b12f Compare July 6, 2023 10:49
@hardfist
Copy link
Contributor

hardfist commented Jul 6, 2023

i think runtime check maybe better than postinstall check,postinstall causes problems in large monorepo,the postinstall costs a lot in monorepo if it has multi versions of rspack which we met when use esbuild in monorepo

@lippzhang
Copy link
Contributor Author

i think runtime check maybe better than postinstall check,postinstall causes problems in large monorepo,the postinstall costs a lot in monorepo if it has multi versions of rspack which we met when use esbuild in monorepo

I think you are right, so where is the right place to put the check? When before createCompiler() , or before compiler build?

@lippzhang lippzhang force-pushed the feature/lippzhang-bettererror branch 3 times, most recently from e4d0e8a to 4a37dd8 Compare July 10, 2023 03:02
@lippzhang lippzhang force-pushed the feature/lippzhang-bettererror branch 2 times, most recently from a030f34 to b3a3c33 Compare July 11, 2023 02:18
@hardfist
Copy link
Contributor

hardfist commented Jul 11, 2023

I am not sure we should check mismatch between core and binding or binding with binding-xx-yy,@h-a-n-a can you help check

@lippzhang lippzhang force-pushed the feature/lippzhang-bettererror branch from b3a3c33 to 1fd012a Compare July 11, 2023 07:47
@h-a-n-a
Copy link
Collaborator

h-a-n-a commented Jul 12, 2023

I am not sure we should check mismatch between core and binding or binding with binding-xx-yy,@h-a-n-a can you help check

@hardfist
This PR mainly focuses on the version consistency between binding and core, also binding and its corresponding addon. So we should put binding and its addon into consideration too.

This change adds a boundary between raw binding output and the real user-end. So we can introduce a middle place for us to do some platform/arch mismatch diagnosis/error report.

cc @lippzhang

Note:
In order to do consistency check, binding should be loaded lazily. You can change the follow import into type-only import(make sure to modify other places too)

import * as binding from "@rspack/binding";

and load the binding lazily. Please deal with the error if any inconsistency happens.

new binding.Rspack(

This file is automatically generated by NAPI-RS. Please leave this file untouched.
https://github.com/web-infra-dev/rspack/blob/main/crates/node_binding/binding.js

@lippzhang lippzhang force-pushed the feature/lippzhang-bettererror branch from 1fd012a to db4ff66 Compare July 12, 2023 07:23
@lippzhang
Copy link
Contributor Author

I can probably understand, but not sure, please help check it @h-a-n-a

@lippzhang lippzhang force-pushed the feature/lippzhang-bettererror branch 2 times, most recently from cc81131 to 268702c Compare July 13, 2023 08:20
@h-a-n-a

This comment was marked as outdated.

@rspack-bot

This comment was marked as outdated.

@h-a-n-a

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@h-a-n-a
Copy link
Collaborator

h-a-n-a commented Jul 19, 2023

The main point of lazily checking is to make sure directly importing @rspack/core will not have any problem, even if users might not installed the corresponding @rspack/binding-xxxx(i.e addon), making sure the consistency is everything for treating @rspack/core and its dependencies as if it's a single library.

type decorated import is not that necessary. As version mismatch is more likely to happen but forgetting to install @rspack/binding-xxxx is not that likely to happen.

@hardfist hardfist changed the title chroe(packages/rspack): add version check for binding chore(packages/rspack): add version check for binding Jul 19, 2023
hardfist
hardfist previously approved these changes Jul 19, 2023
@h-a-n-a

This comment was marked as outdated.

@rspack-bot

This comment was marked as outdated.

@h-a-n-a h-a-n-a changed the title chore(packages/rspack): add version check for binding feat(packages/rspack): add version check for binding Jul 20, 2023
@h-a-n-a
Copy link
Collaborator

h-a-n-a commented Jul 20, 2023

!eco-ci

@h-a-n-a
Copy link
Collaborator

h-a-n-a commented Jul 20, 2023

!canary

@rspack-bot
Copy link

rspack-bot commented Jul 20, 2023

📝 Ran ecosystem CI: Open

suite result
modernjs ✅ success
nx ✅ success

@github-actions
Copy link
Contributor

0.2.8-canary-e979e23-20230720051633

@h-a-n-a h-a-n-a added this pull request to the merge queue Jul 20, 2023
@h-a-n-a h-a-n-a removed this pull request from the merge queue due to a manual request Jul 20, 2023
@h-a-n-a h-a-n-a added this pull request to the merge queue Jul 20, 2023
Merged via the queue into web-infra-dev:main with commit 7778594 Jul 20, 2023
IWANABETHATGUY pushed a commit that referenced this pull request Jul 20, 2023
* chore(packages/rspack): add version check for binding

* feat: optimize version check

* chore: cleanup

* feat: optimize impl

* fix: remote version check

---------

Co-authored-by: Hana <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add version check for binding
6 participants