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 137 forks source link

feat/Clean build to have ES2018 as main version #129

Closed akanass closed 3 years ago

akanass commented 3 years ago

This PR updates @simplewebauthn/browser's build and bundling pipeline to generate artifacts to support a wider range of use cases in a simplified way to use. This includes:

A new "main" build that targets ES2018 A new "unpkg" entry in "package.json" to reference UMD bundle that targets ES2018 UMD bundle that targets ES2018 UMD bundle that targets ES5

The documentation has been updated as well to explain ES2018 is now the only version and how to use UMD bundle in a simplified way.

MasterKale commented 3 years ago

The only bundle I want to get rid of, based on the discussion in #128, is the ES5 CJS bundle. The ideal end state for @simplewebauthn/browser bundles breaks down thusly:

The ES5 build step still needs to happen, but with only the umd build defined. "main" should then be updated to refer to the ES2018 bundle to align it with what'll get defined as "module" once #128 gets merged.

akanass commented 3 years ago

@MasterKale I will update my PR and get back the ES5 UMD bundle.

But I don't understand why we still need the module entry? If we have the ES2018 in main entry it's enough else you will put the same version in both entries.

akanass commented 3 years ago

@MasterKale I've updated all

MasterKale commented 3 years ago

But I don't understand why we still need the module entry? If we have the ES2018 in main entry it's enough else you will put the same version in both entries.

Can we be sure "main" is recognized by all modern bundlers just as "module" now seems to be? In my mind we'd specify both but if everything will recognize "main" when "module" is missing (understanding that the standard main property came before module) then I'm fine with this PR.

akanass commented 3 years ago

@MasterKale I think only this PR is required and we can close #128

akanass commented 3 years ago

@MasterKale The main entry is the standard and the only one that exists natively.

Using module is to override the main and in our case, we don't need it because we would put the same version in both.

The problem with Vite was that it took the ES5 version so the one that was in the main since we don't have a module.

This is the proof that main is the standard data which is taken by all build systems.

skoshx commented 3 years ago

@MasterKale @akanass My concern is that some bundlers won't recognise the "main" entry as an ES module, and that bundlers will assume that it's CJS format. In most packages, they have a "main" CJS entry, and a "module" ES module entry, so I think bundlers might make that distinction too… Anyways, it might be a good idea to just make sure that everything works with having an ES module as "main".

akanass commented 3 years ago

@skoshx I don't agree with you because in main entry you can put what you want.

You only use module to have ESM version if you have CJS in main.

In our case main is the ESM and it's enough.

I did it with my own library which was built like here before and I did the same updates then put it in a project and compile it in ESNEXT and all works like a charm.

Build system will always look at the main if nothing else is declared because it's the only native entry.

For me all is good like this.

akanass commented 3 years ago

And as I always said, bundler are always taking the files in the order of mainFields which is ["module","main"] by default

So if you don't have module entry the main will be taken

skoshx commented 3 years ago

Right… Anyways, good to see this PR merged!

MasterKale commented 3 years ago

@MasterKale @akanass My concern is that some bundlers won't recognise the "main" entry as an ES module, and that bundlers will assume that it's CJS format. In most packages, they have a "main" CJS entry, and a "module" ES module entry, so I think bundlers might make that distinction too… Anyways, it might be a good idea to just make sure that everything works with having an ES module as "main".

@skoshx Good looking out for this use case. I did some independent testing just now with the ES2018 ESM bundle as "main" across my "integration test suite" consisting of the following:

I'm happy to report that all of these frameworks handled the ES2018 bundle as "main" no problem, and were all able to be built down to ES5 to function as expected in IE10.

@akanass Thank you for your contribution to the cause. Here's to SimpleWebAuthn remaining simple to use going forward :v:

akanass commented 3 years ago

@MasterKale Thank you for your trust and I am happy to have been able to improve our previous contribution and thank you also @skoshx for guiding us on it

I'm just disappointed that we went in the wrong direction, thinking that we needed an ES5 version at all costs, I should have tested better with the @Moumouls repo

The main thing is that we have something simple as the name carries it so well

akanass commented 3 years ago

@MasterKale I saw your new release and maybe you should have made a major version because there is still a breaking change in the build system and the main version has been changed.

Anyway for my library that's what I did, here it was just like that lol

MasterKale commented 3 years ago

I did indeed just cut a new release, @simplewebauthn/browser@3.1.0 includes this PR.

...you should have made a major version because there is still a breaking change in the build system and the main version has been changed.

I guess we'll see who this breaks 🙃

akanass commented 3 years ago

@MasterKale I meant having 4.0.0 release but it's your call this choice :)