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: downgrade ts target to support old browsers #112

Closed Moumouls closed 3 years ago

Moumouls commented 3 years ago

On our production systems we use supportsWebauthn fn.

Sadly it seems on old browser (like IE or old Edge) the fn makes the whole app to crash since the package is compiled with too high ES version. (spread operators in min.js ... seems to be the root cause)

A target on ES5 should be sufficient to allow feature detection to works on IE11, old Edge

akanass commented 3 years ago

The solution is not to change the build version but to use this one is to either recompile your own bundle or to use SystemJS to import into old versions. This last solution works because I do it for me we import either modules for browsers that support it or with SystemJS.

Afterwards, a build of different versions can be set up to have the support of different browsers but in no case to have only the support of the old ones because this would weigh down the bundle with unnecessary things

Moumouls commented 3 years ago

Yes i know it's possible to fork and recompile but i'm convinced that this lib should works without additional systems on old browsers. In other hand; to avoid a global "weight down" and enhance DX (just import and it works); a specific new "support" package compiled with ES5/ES3 could be interesting.

Then in front frameworks like Next.js, the system will load the "support" package first then if browser do not support webauthn, the webauthn code will not be imported or executed

MasterKale commented 3 years ago

@Moumouls What kinds of errors get thrown in IE11 and Edge Legacy? How are you attempting to consume @simplewebauthn/browser in these older browsers? And does this target downgrade definitively solve the issue?

Can you include a comparison of build sizes? I'm also a fan of screenshots or screencasts of the fix in action.

I appreciate the irony of a library compiled with too-new a build target that its "does this work in older browsers" breaks in those older browsers, but I do wonder what issues are actually arising. Maybe it's just an issue with the way the global method of use is defined? Or perhaps it's a const/let vs var issue. Maybe the solution is a Webpack configuration change rather than a TypeScript target downgrade?

Moumouls commented 3 years ago

Hi @MasterKale my env is a simple NextJS app with TS target on ES5. i've some custom webpack config but just to resolve aliases correctly.

I used browerstack https://www.browserstack.com/ with a IE11 and the old MS Edge (not the chrome based one) to reproduce the error.

The issue come from the spread {...e in min.js

Capture d’écran 2021-03-30 à 16 34 11

Capture d’écran 2021-03-30 à 16 34 22

You can find the {...e occurence into @simplewebauthn/browser/dist/simplewebauthn-browser.min.js

Moumouls commented 3 years ago

i'm sure that i can fix this with some additional webpack config, but it seems weird to me to have a package with a support function check that makes the app crash (feature detection is commonly used at app start), when we attempt to call the support function to... check if the browser support the feature ahaha 🙂

Moumouls commented 3 years ago

a first move could be just to downgrade the ES target then a second move to create a specific package support with a target ES5/ES3 and restore ES 2018 on all other packages

akanass commented 3 years ago

I am not a fan to downgrade the compilation version and only have the es5 version.

We can have multiple version in the same bundle with different entries inside package.json to resolve them but please not only one version because we will impose an es5 version to everyone and it's not good.

In my case I don't have any problems to use this library with all browsers and I have an import in module and another one with SystemJS like this all browsers are supported.

I am using rollup to build my project and all is working well.

So please don't consider to only have es5 version but having multiple entries in package.json for the lib or you have to build your project where you're using this library with better webpack options but this library is working well.

Another point is the support of Webauthn in old browser and IE11 doesn't support it apparently.

You can check all browsers support here https://developers.yubico.com/WebAuthn/WebAuthn_Browser_Support/

MasterKale commented 3 years ago

Ah, the ol' spread operator. This isn't the first time I've had to deal with IE breaking because of that. It should be simple enough to fix as that doesn't even need a polyfill, just an older target as you're proposing.

i'm sure that i can fix this with some additional webpack config, but it seems weird to me to have a package with a support function check that makes the app crash (feature detection is commonly used at app start), when we attempt to call the support function to... check if the browser support the feature ahaha 🙂

You're right that it shouldn't become a burden of the dev using a library to tailor their build process to accommodate the library. I have no problems updating @simplewebauthn/browser, I just wanted to know what the problem was before I endorsed any fix for it.

I am not a fan to downgrade the compilation version and only have the es5 version.

I agree with this, though.

@Moumouls: I can't accept this PR as-is because now that I'm at my desk I see that you're setting the target of the entire monorepo to ES5, not just @simplewebauthn/browser. This PR should be restricted to packages/browser/tsconfig.json instead.

MasterKale commented 3 years ago

For sake of comparing impact on build sizes, here are some numbers:

Target: ES2018

simplewebauthn-browser.min.js 4.43 KiB

Hash: a1c8cacd19711dff75a1
Version: webpack 4.46.0
Time: 1615ms
Built at: 03/30/2021 8:35:32 AM
                                       Asset       Size  Chunks                   Chunk Names
        helpers/base64URLStringToBuffer.d.ts  351 bytes          [emitted]        
        helpers/bufferToBase64URLString.d.ts  328 bytes          [emitted]        
               helpers/supportsWebauthn.d.ts  112 bytes          [emitted]        
helpers/toPublicKeyCredentialDescriptor.d.ts  226 bytes          [emitted]        
                   helpers/toUint8Array.d.ts  194 bytes          [emitted]        
                                  index.d.ts  432 bytes          [emitted]        
                 methods/startAssertion.d.ts  409 bytes          [emitted]        
               methods/startAttestation.d.ts  430 bytes          [emitted]        
               simplewebauthn-browser.min.js   4.43 KiB       0  [emitted]        main
           simplewebauthn-browser.min.js.map   15.9 KiB       0  [emitted] [dev]  main
Entrypoint main = simplewebauthn-browser.min.js simplewebauthn-browser.min.js.map
[0] ./src/helpers/base64URLStringToBuffer.ts 1.2 KiB {0} [built]
[1] ./src/helpers/supportsWebauthn.ts 361 bytes {0} [built]
[2] ./src/helpers/bufferToBase64URLString.ts 682 bytes {0} [built]
[3] ./src/helpers/toPublicKeyCredentialDescriptor.ts 538 bytes {0} [built]
[4] ./src/index.ts 685 bytes {0} [built]
[5] ./src/methods/startAttestation.ts 2.51 KiB {0} [built]
[6] ./src/helpers/toUint8Array.ts 315 bytes {0} [built]
[7] ./src/methods/startAssertion.ts 2.57 KiB {0} [built]
lerna success Bootstrapped 1 package

Target: ES5 w/downlevelIteration

simplewebauthn-browser.min.js 8.55 KiB

Hash: dc3a2546839df0a2a4b8
Version: webpack 4.46.0
Time: 1892ms
Built at: 03/30/2021 8:31:18 AM
                                       Asset       Size  Chunks                   Chunk Names
        helpers/base64URLStringToBuffer.d.ts  351 bytes          [emitted]        
        helpers/bufferToBase64URLString.d.ts  328 bytes          [emitted]        
               helpers/supportsWebauthn.d.ts  112 bytes          [emitted]        
helpers/toPublicKeyCredentialDescriptor.d.ts  226 bytes          [emitted]        
                   helpers/toUint8Array.d.ts  194 bytes          [emitted]        
                                  index.d.ts  432 bytes          [emitted]        
                 methods/startAssertion.d.ts  409 bytes          [emitted]        
               methods/startAttestation.d.ts  430 bytes          [emitted]        
               simplewebauthn-browser.min.js   8.55 KiB       0  [emitted]        main
           simplewebauthn-browser.min.js.map   16.1 KiB       0  [emitted] [dev]  main
Entrypoint main = simplewebauthn-browser.min.js simplewebauthn-browser.min.js.map
[0] ./src/helpers/base64URLStringToBuffer.ts 1.19 KiB {0} [built]
[1] ./src/helpers/supportsWebauthn.ts 361 bytes {0} [built]
[2] ./src/helpers/bufferToBase64URLString.ts 1.5 KiB {0} [built]
[3] ./src/helpers/toPublicKeyCredentialDescriptor.ts 916 bytes {0} [built]
[4] ./src/index.ts 679 bytes {0} [built]
[5] ./src/methods/startAttestation.ts 5.85 KiB {0} [built]
[6] ./src/helpers/toUint8Array.ts 315 bytes {0} [built]
[7] ./src/methods/startAssertion.ts 6.02 KiB {0} [built]
lerna success Bootstrapped 1 package

Almost a 2x increase in build size, but will anyone really feel an additional 4KB of download?

akanass commented 3 years ago

@MasterKale yes I feel of this additional size especially if you're calling the website on a mobile with a poor connection.

And using only es5 will not allow a good module import in modern browser and all webauthn compatible browser supports ES2018 compilation like explain in the other PR

So please don't use es5 as compilation version.

Even modern framework like Angular is compiling in ES2018 and they stop using ES5 so why this library should do it especially if modern browser can use ES2018.