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

chore!: refactor wasm loading for analyze #1832

Merged
merged 21 commits into from
Dec 13, 2024
Merged

Conversation

e-moran
Copy link
Contributor

@e-moran e-moran commented Oct 2, 2024

Refactors wasm loading so that the analyze package only exports an initialisation function. This greately reduces the amount of code duplication, because we don't need functions per-wasm function per-runtime.

Closes #1448

Copy link

trunk-io bot commented Oct 2, 2024

😎 Merged successfully - details.

@e-moran e-moran requested a review from blaine-arcjet October 2, 2024 16:45
@e-moran e-moran marked this pull request as ready for review October 2, 2024 16:45
@e-moran e-moran requested a review from a team as a code owner October 2, 2024 16:45
Copy link
Contributor

@blaine-arcjet blaine-arcjet left a comment

Choose a reason for hiding this comment

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

I don't want this logic only available via rules. The linked issue says we need to create an @arcjet/analyze-wasm package.

@e-moran
Copy link
Contributor Author

e-moran commented Oct 3, 2024

@blaine-arcjet Just to clarify before I make the changes. You're proposing we have an analyze package with roughly the same public api as before, an analyze-wasm package that just provides the initialise wasm function. And then the arcjet package should consume only the analyze package in roughly the same way as it did before?

@blaine-arcjet
Copy link
Contributor

Just to clarify before I make the changes. You're proposing we have an analyze package with roughly the same public api as before, an analyze-wasm package that just provides the initialise wasm function. And then the arcjet package should consume only the analyze package in roughly the same way as it did before?

Correct. I expect that arcjet should not need any changes.

@e-moran
Copy link
Contributor Author

e-moran commented Oct 3, 2024

Just to clarify before I make the changes. You're proposing we have an analyze package with roughly the same public api as before, an analyze-wasm package that just provides the initialise wasm function. And then the arcjet package should consume only the analyze package in roughly the same way as it did before?

Correct. I expect that arcjet should not need any changes.

Great! I'll update this PR to do that then.

Copy link

socket-security bot commented Oct 15, 2024

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@arcjet/[email protected] None 0 0 B

View full report↗︎

@blaine-arcjet
Copy link
Contributor

@e-moran one thing I wanted to flag here is that I didn't see a sibling PR to change the build scripts in our analyze packages. We'll want both of them submitted before we merge this.

Copy link
Contributor

@blaine-arcjet blaine-arcjet left a comment

Choose a reason for hiding this comment

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

Still needs some cleanup. I also expressed some thoughts on how we could reduce some more boilerplate.

.github/release-please-config.json Outdated Show resolved Hide resolved
analyze-wasm/CHANGELOG.md Outdated Show resolved Hide resolved
analyze-wasm/README.md Outdated Show resolved Hide resolved
analyze-wasm/README.md Outdated Show resolved Hide resolved
analyze-wasm/README.md Outdated Show resolved Hide resolved
analyze/README.md Show resolved Hide resolved
analyze/README.md Outdated Show resolved Hide resolved
analyze/package.json Outdated Show resolved Hide resolved
analyze/package.json Outdated Show resolved Hide resolved
analyze/package.json Outdated Show resolved Hide resolved
analyze/index.ts Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
analyze/index.ts Outdated Show resolved Hide resolved
@trunk-io trunk-io bot merged commit 02e4435 into main Dec 13, 2024
29 checks passed
@trunk-io trunk-io bot deleted the eoin/refactor-wasm-loader branch December 13, 2024 17:05
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.

Refactor analyze into a wasm loader package and a logic package
2 participants