MasterKale / SimpleWebAuthn

WebAuthn, Simplified. A collection of TypeScript-first libraries for simpler WebAuthn integration. Supports modern browsers, Node, Deno, and more.
https://simplewebauthn.dev
MIT License
1.63k stars 138 forks source link

Fix Issue 114 - Rollup Configuration + TSCONFIG Files #116

Closed akanass closed 3 years ago

akanass commented 3 years ago

I did all the configuration for rollup and the typescript compiler. All builds are working and the produced files are correct.

I just have the problem with the Microsoft comment in the bundle file because it is added on the fly and I don't know where it comes from because I don't have it in my projects.

We will have to dig a little more, but I don't have more time today to do it.

The main thing is that everything is working and that the results are correct despite this intrusive comment.

akanass commented 3 years ago

I will try to find later the problem of the comment so you can let this PR open or I will create a new one if I find the problem.

I will maybe remove few plugins as well from the rollup configuration because you're not calling externals library and like this the file will be more clean even if this configuration is the most complete and is a good example for you

MasterKale commented 3 years ago

Thanks for submitting this, there's a single line in here that fixed the issue I was having of the ES5 bundle not resolving anything in the imports:

commonjs({ extensions: ['.js', '.ts'] }),

Once I pulled this into my set up the bundle built as expected. I think with this addition to my setup I'm that much closer to a PR for this. I'll clean up in there and then submit a PR with everything together.

akanass commented 3 years ago

@MasterKale this is not only the commonjs plugin but the configuration of typescript as well.

You have to take them too because they are good like this.

Let me finished the thing later because I can optimised the build with only rollup as we only want the versions for browser

akanass commented 3 years ago

I'll do my other tests later so can you wait on it please

akanass commented 3 years ago

@MasterKale

I have finished all of the optimizations and the builds are much better now. I only used rollup and the result is very nice.

We have a unique file for each version ES2018, ES5 and UMD.

The ES5 and UMD versions are optimized to the maximum and they will need to have tslib installed to use them but everyone using typescript has this library.

By putting tslib externally, we not only gain a lot of weight but above all we avoid code redundancy which is really not good for user builds.

Their build system will take care of injecting tslib during the final compilation or it will leave it externally which would be even better.

All my projects work this way because the use of tslib must be external to the build of a library if we want to optimize at best and we want compatibility with old browsers.

It will therefore be necessary to indicate in the documentation that the users who want the ES5 version will have to have tslib installed and I put it in peerDependencies so that it will take that of the end user.

It is the same for the UMD version, it will be necessary that the user also imports tslib because it is ES5.

But for the UMD version, we could put it in ES2108 and we would be safe because people who use this version should have a compatible browser otherwise they will take the ES5 version.

It would make a lot more sense to put the UMD in ES2108.

If it's okay with you, I make the change immediately.

There you go, I hope you enjoy the changes and the end result which is very good for a FRONT library, we just have to put UMD in ES2018 and that would be perfect.

akanass commented 3 years ago

UMD version with ES2018 as target is ready and much better, I can commit it when you want especially since your UMD bundle was in ES2018 before so it really makes more sense to have this version because we have the other bundles now to be used in modern or old browsers.

And with this UMD version, we have less weight and and no need for tslib

MasterKale commented 3 years ago

@akanass Interesting strategy using Rollup for everything. I think you can even get it down to a single rollup.config.js if you export default [{ ...rollup.config.js }, { ...rollup.config.es5.js }]; from rollup.config.js. At that point the build process distills down to rimraf dist && rollup -c and we remove even more build dependencies...I can take care of this after this gets merged :v:

It will therefore be necessary to indicate in the documentation that the users who want the ES5 version will have to have tslib installed and I put it in peerDependencies so that it will take that of the end user.

What does this doc update look like? Something like this? (I'm just brainstorming this)

If you need to support IE11, npm install tslib into your project and set "target": "es5" into your tsconfig.json. Also make sure to set "web" target in Webpack/Rollup to grab the "browser" bundle defined in @simplewebauthn/browser's package.json.

Based on the fact that TypeScript pulled tslib into the build bundle automatically, I wonder if a developer using this library would even need to manually install it, as opposed to just setting "target": "es5". You have the most experience with this, though, so I have to defer to you.

Also I'm fine to merge this, but there are merge conflicts. Would you like to take a stab at resolving them?

akanass commented 3 years ago

@MasterKale of course I will resolve the conflicts and check for the single configuration file.

What do you think for the UMD version ? Can we do it targeting ES2018? I think it's the better way to do it.

And I will explain the documentation as well.

Thank you for your acceptance of this PR when all will be done.

MasterKale commented 3 years ago

What do you think for the UMD version ? Can we do it targeting ES2018? I think it's the better way to do it.

Honestly I'd like to offer two UMD bundles, one that targets ES2018 and another that targets ES5. I have consumed @simplewebauthn/browser more as a UMD via unpkg in <script> tags than I have as an ESM imported into a React/etc... SPA. It's been the simplest way to launch WebAuthn calls as I complete work on some other WebAuthn backend work. No Create-React-App, just create an HTML page, add a script tag, and forget about the front end.

I think it would be great to continue to offer that flexibility going forward. A server-rendered webpage should have as much freedom to choose to target modern browsers only, via the ES2018 UMD, or or target older browsers by linking to the ES5 UMD. A SPA architecture shouldn't be a prerequisite to achieving that.

akanass commented 3 years ago

@MasterKale I will do both versions so.

But for UMD targetingES5, tslib will be included inside the bundle if you want to use it in a simple tag script.

The bundle will be heavier but we don't have the choice for this target.

I will create a little rollup plugin to clean the tslib header which is always injected like this the bundle will be perfect.

You will have all very soon.

akanass commented 3 years ago

@MasterKale all is done.

conflict resolution, build optimizations, documentation and file cleanup

MasterKale commented 3 years ago

@akanass This is an awesome contribution to this effort, thank you very much! I also appreciate you taking the time to document everything in the README 🙇‍♂️

akanass commented 3 years ago

@MasterKale it's my pleasure :D I liked to do it and helped you on it.

I'll continue in the future to contribute on this project if I can add more value.

Thank you for accepting and merging this PR.