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/Clean build to have ES2018 as main version #129

Merged
merged 7 commits into from
Jun 18, 2021
Merged

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

merged 7 commits into from
Jun 18, 2021

Conversation

akanass
Copy link
Contributor

@akanass akanass commented Jun 17, 2021

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
Copy link
Owner

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:

  • ES2018: ESM + UMD
  • ES5: UMD

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
Copy link
Contributor Author

akanass commented Jun 17, 2021

@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
Copy link
Contributor Author

akanass commented Jun 17, 2021

@MasterKale I've updated all

@MasterKale MasterKale added the package:browser @simplewebauthn/browser label Jun 17, 2021
@akanass akanass changed the title fix/Clean build version to only have ES2018 feat/Clean build version to only have ES2018 Jun 17, 2021
@akanass akanass changed the title feat/Clean build version to only have ES2018 feat/Clean build version to have ES2018 as main version Jun 17, 2021
@akanass akanass changed the title feat/Clean build version to have ES2018 as main version feat/Clean build to have ES2018 as main version Jun 17, 2021
@MasterKale
Copy link
Owner

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
Copy link
Contributor Author

akanass commented Jun 17, 2021

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

@akanass
Copy link
Contributor Author

akanass commented Jun 17, 2021

@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
Copy link

skoshx commented Jun 17, 2021

@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
Copy link
Contributor Author

akanass commented Jun 17, 2021

@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
Copy link
Contributor Author

akanass commented Jun 17, 2021

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
Copy link

skoshx commented Jun 18, 2021

Right… Anyways, good to see this PR merged!

@MasterKale
Copy link
Owner

MasterKale commented Jun 18, 2021

@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:

  • Next.js + JS
  • Next.js + TS
  • Parcel v2.0
  • Create-React-App + JS
  • Create-React-App + JS

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 ✌️

@MasterKale MasterKale merged commit 99aed7e into MasterKale:master Jun 18, 2021
@akanass
Copy link
Contributor Author

akanass commented Jun 18, 2021

@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
Copy link
Contributor Author

akanass commented Jun 18, 2021

@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
Copy link
Owner

MasterKale commented Jun 18, 2021

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
Copy link
Contributor Author

akanass commented Jun 18, 2021

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

@akanass akanass deleted the clean-build-version branch June 19, 2021 16:16
@MasterKale MasterKale mentioned this pull request Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:browser @simplewebauthn/browser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants