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(harper.js): export both binary and inlinedBinary for different runtimes #607

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

Asuka109
Copy link
Contributor

@Asuka109 Asuka109 commented Feb 6, 2025

This pull request includes several changes to improve the Harper.js project, focusing on updating dependencies, modifying test configurations, and refactoring the code to use the new harper-wasm package. The most important changes include updating the package.json dependencies, modifying the test scripts, and refactoring the Linter and LocalLinter classes to use the new binary module.

Dependency and configuration updates:

  • packages/harper.js/package.json: Updated dependencies to use harper-wasm instead of wasm, added new test scripts, and updated the versions of several development dependencies. Added sideEffects field so that all the exports can be removed by bundler treeshaking.

Refactoring and code improvements:

  • packages/harper.js/src/binary.ts: Migrated from src/WorkerLinter/communication.ts and src/loadWasm.ts, it provides BinaryModule as a wrapper for the raw WebAssembly module, bringing more readable method names and additional features. It also exports binary and binaryInlined to load the WebAssembly file in different ways.
  • packages/harper.js/src/Linter.ts: Updated imports to use harper-wasm and added a new LinterInit interface that includes the binary module. [1] [2]
  • packages/harper.js/src/LocalLinter.ts: Refactored the LocalLinter class to use the new binary module for WebAssembly operations, removing the old loadWasm method. Now it should be initialized with passing a BinaryModule.
  • packages/harper.js/src/WorkerLinter/index.ts: Updated the WorkerLinter class to use the new binary module and refactored the worker communication setup.

Testing improvements:

@Asuka109 Asuka109 marked this pull request as draft February 6, 2025 16:40
@elijah-potter
Copy link
Collaborator

Hey @Asuka109, just a heads up. If you want the precommit job to ✔️, you need to run just format and commit the results.

@Asuka109
Copy link
Contributor Author

Asuka109 commented Feb 13, 2025

@elijah-potter Got it. I missed your comment last week. I tried it just now, but it seems it won't resolve the ESLint issues. Also, I noticed that VSCode cannot resolve the .eslintrc file and isn't linting the problems. I can improve that in the next PR.

@Asuka109
Copy link
Contributor Author

Asuka109 commented Feb 13, 2025

@elijah-potter It seems like there is something wrong with the cases running in Firefox. I will handle it later. There are quite a lot of changes to the API. Could you please take a look at the changes and let me know if anything needs to be changed?

Copy link
Collaborator

@elijah-potter elijah-potter left a comment

Choose a reason for hiding this comment

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

You've made a lot of changes here. Some of the reasoning needs a bit more explanation.

packages/harper.js/src/utils.ts Outdated Show resolved Hide resolved
packages/harper.js/src/binary.ts Show resolved Hide resolved
packages/harper.js/src/LocalLinter.ts Show resolved Hide resolved
@Asuka109 Asuka109 marked this pull request as ready for review February 15, 2025 12:33
@Asuka109
Copy link
Contributor Author

@elijah-potter I think everything is ready now.

@Asuka109
Copy link
Contributor Author

@elijah-potter I found that initializing binary makes Node.js programs panic, even when only binaryInlined is used. Please hold off on reviewing this until I fix it.

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.

2 participants