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.65k stars 138 forks source link

fix: downgrade ts version on browser pkg #113

Closed Moumouls closed 3 years ago

Moumouls commented 3 years ago

Here we are @MasterKale

jstewmon commented 3 years ago

I think this should be a semver major bump if merged, as it may be a breaking change due to instanceof checks ceasing to work.

https://github.com/Microsoft/TypeScript/wiki/FAQ#why-doesnt-extending-built-ins-like-error-array-and-map-work

akanass commented 3 years ago

@MasterKale please don't compile browser package only in es5 it's not good for people who wants to use module in modern browser.

If this is the solution used, I will stop to use this package because I don't want to be limited with es5 compilation. I am using moduleimport in my browser with a fallback to SystemJS and all is working with all browsers which supports WebAuthn.

And like I said before, if we want to have es5 support we need multiple entries in package.json with multiple builds for the library, even if I think it's not required because IE11 doesn't support WebAuthn so what is exactly the purpose of that?

Yubico website provides the list of supported browser just here and you can see only Edge is supported and this browser support ES2018 compilation.

The compilation in es5 isn't required with my point of view and if you really want it, don't do it as a standard but with multiple compilation in the bundle.

Thanks to take care of this advice.

mariusmarais commented 3 years ago

This leaves a bad taste in the mouth. None of these functions work in such very old browsers, now everyone has to pay the price?

A simple Webpack rule would be sufficient to repack this module from node_modules for those that need to support the unsupported?

Alternatively feature-detection can be run before trying to load the module and then skip it? Or use something like SystemJS?

Quite frankly there are many options for those that need to support such old environments (where this package doesn't work at all) without dragging everyone down.

akanass commented 3 years ago

@mariusmarais I totally agree with you, we shouldn’t be impacted by old browser supports especially if those browsers don’t support WebAuthn.

And even if it'll be the case, the library should be stay like this and people who want to integrate it in old browser, should use a better compilation on their own or use SystemJS like you and I said

MasterKale commented 3 years ago

Let's cool our jets, people, I'm picking up some misdirected enthusiasm for the topic of how you support feature detection in old browsers without saddling the library with too much unnecessary polyfills/"downlevels"/etc...

This is a unique scenario in which some code needs to be transpiled to a lower level to do its job, while the majority of the code is intentionally targeting only newer supported browsers. I think @Moumouls has part of the puzzle, while @mariusmarais's suggestions are another couple pieces. Together we'll get to a solution, while avoiding resorting to things like threatening to "stop using this library" if it changes in some way.

This project is a labor of love for me and those who submit PR's, let's remember that none of us are really getting paid for this. I ask we all please keep this in mind going forward :)

MasterKale commented 3 years ago

...even if I think it's not required because IE11 doesn't support WebAuthn so what is exactly the purpose of that?

The purpose of lowering the TS build target would be to output a bundle that can run in IE/Edge Legacy so that supportsWebAuthn() can actually run to return false.

Yubico website provides the list of supported browser just here and you can see only Edge is supported and this browser support ES2018 compilation. And it was charts like this that justified my originally setting "target": "ES2018" in the root-level tsconfig.json. However, as we see now, @simplewebauthn/browser builds in a state that makes it difficult to work in IE11/Edge Legacy/etc..., negating the reason for supportsWebAuthn's existence.

The path forward is still ambiguous to me. It's likely not as simple as updating packages/browser/tsconfig.json. I'm going to come at this from the Webpack angle - maybe there's a way to build the code that lets developers pick-and-choose which version of the library they want to use in their code. Maybe the solution is another library that's only the supportsWebAuthn() method. A third alternative is I just nuke supportsWebAuthn() and update the docs to say, "if you need to support IE11/Edge Legacy, here's a snippet of VanillaJS you can copy-paste into your code to tell if WebAuthn is available."

I'm open to suggestions. In fact, I'm going to make another Issue for us to continue this discussion as it feels weird to have all this within a PR.

MasterKale commented 3 years ago

Apologies, @Moumouls, but I'm going to close this PR and move the discussion into Issue #114. I know we can figure something out, I just think you accidentally stumbled onto an interesting edge case of building a WebAuthn library for the front end.

akanass commented 3 years ago

@MasterKale First of all, I wanted to apologize if my words were misinterpreted and there was no threat in the air but just a disappointment in the turn of things because it is not because a person is found in a bizarre case or in a specific implementation that the whole community needs to be impacted.

A solution must indeed be found and this is what I have always done for this library because I admire the work you have done and I will always support you because this project pates me a lot.

But when I saw that despite the recommendations and solutions, the unique ES5 compilation was maintained so yes I did not like it and I apologize once again.

This is not the first time that we have entered into specific cases where the library has to be changed for a person and we realize in the end that this is not the right solution.

So I will go to the other issue to give my opinion and my solutions but again, I do not see why we have to compile in ES5 since the old browsers do not support the WebAuthn functionality.