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: use tsup as a bundler #62

Merged
merged 4 commits into from
Feb 15, 2024
Merged

Conversation

cre8
Copy link
Contributor

@cre8 cre8 commented Feb 13, 2024

Instead of tsc, it using https://tsup.egoist.dev/ to generate commonjs and esm

Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
@lukasjhan
Copy link
Contributor

I'll test the bundle and the browser testing :)

@lukasjhan
Copy link
Contributor

@cre8 I got a quick question.

When I build the package with tsup, tsup concat all the code into one js file and one d.ts file. (or one mjs and one d.mts, I guess this is ESM). Can I still get the benefits of tree shaking even if I only have one file?

@cre8
Copy link
Contributor Author

cre8 commented Feb 14, 2024

@lukasjhan Tree shaking works great with ESM, since it can analyse the exports and remove unused code before bundling. Therefore putting everything will not have a negative impact on the build size, see: https://esbuild.github.io/api/#tree-shaking
It also has no negative side for debugging since we are adding the .d.ts files.

tsub however has a "legacy" mode that will create a separate folder for esm and cjs and compile every file for its own: https://tsup.egoist.dev/#bundle-formats

@lukasjhan
Copy link
Contributor

@lukasjhan Tree shaking works great with ESM, since it can analyse the exports and remove unused code before bundling. Therefore putting everything will not have a negative impact on the build size, see: https://esbuild.github.io/api/#tree-shaking It also has no negative side for debugging since we are adding the .d.ts files.

tsub however has a "legacy" mode that will create a separate folder for esm and cjs and compile every file for its own: https://tsup.egoist.dev/#bundle-formats

Good :) I'm currently resolved conflict and fixing tests on the top of your branch.
You can check my progress here: https://github.com/lukasjhan/sd-jwt-js/tree/feat/esm%2Bcjs-fix

@lukasjhan
Copy link
Contributor

This is really strange in here https://github.com/lukasjhan/sd-jwt-js/tree/feat/esm%2Bcjs-fix.
I'm still using node crypto in test files, but test:broswer succeed.

Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
@cre8
Copy link
Contributor Author

cre8 commented Feb 14, 2024

@lukasjhan I found no way to use window.subtle function inside the jsdom, it seems that is not integrated. Right now I am building a dummy web app to test it there directly to verify to correctness in the browser

@cre8
Copy link
Contributor Author

cre8 commented Feb 14, 2024

@lukasjhan works nice in the browser with esm and the test are passing, see: https://github.com/cre8/sd-jwt-test

Copy link
Contributor

@lukasjhan lukasjhan left a comment

Choose a reason for hiding this comment

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

@cre8 Wow I tried your test repo, It tests with the broswer.
Maybe jsdom just mock the DOM, not the whole broswer. We can try to integrate broswer later :)

I think PR is good

@lukasjhan lukasjhan merged commit e1ae323 into openwallet-foundation:main Feb 15, 2024
2 checks passed
@cre8
Copy link
Contributor Author

cre8 commented Feb 15, 2024

Since we are not using window.crypto in the actual code base, it should be fine that it is not covered by jsdom and we can still achieve 100%.
In case we include it, we can mock it for the test (or look for a parallel test that is supporting browser, but for now jsdom should be fine)

@cre8 cre8 deleted the feat/esm+cjs branch February 15, 2024 06:28
cre8 added a commit to cre8/sd-jwt-js that referenced this pull request Mar 8, 2024
Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
Co-authored-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
cre8 added a commit that referenced this pull request Mar 8, 2024
Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
Co-authored-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
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