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

What to do about supportsWebAuthn()... #114

Closed
MasterKale opened this issue Mar 30, 2021 · 95 comments
Closed

What to do about supportsWebAuthn()... #114

MasterKale opened this issue Mar 30, 2021 · 95 comments
Labels
package:browser @simplewebauthn/browser

Comments

@MasterKale
Copy link
Owner

As highlighted in PR #112 and #113, @simplewebauthn/browser's supportsWebAuthn() method can't actually execute in browsers that don't support WebAuthn, like IE11 and Edge Legacy, because those browsers don't support ES2018 syntax. This negates the reason for its existence.

As I mentioned in PR #113, the path forward is ambiguous to me. It's likely not as simple as updating packages/browser/tsconfig.json to target ES5 instead of ES2018.

Possible options I proposed included:

  • Maybe there's a way to build the code with Webpack 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 built for ES5 environments.
  • 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 on how we might solve the problem of building a library that simultaneously needs to sometimes support ES5 while often supporting ES2018.

@MasterKale
Copy link
Owner Author

For the record I'm leaning towards option 3️⃣ because it's the easiest to pull off 😂

@mariusmarais
Copy link

mariusmarais commented Mar 30, 2021

(Trying to be more constructive the second time around...)

It seems that it's possible to build for different environments using babel, e.g. https://stackoverflow.com/a/52024307 (It is, but I think it's off-target in this case.)

Since you're already using webpack, option 1 shouldn't be that hard, but I haven't attempted it myself.
Maybe start with the multiple targets example here https://webpack.js.org/concepts/targets/ and add Babel in ES5 mode in the second target? Add both outputs to the NPM module and users load which they prefer?

Another nice example using webpack-merge, since these configs will almost be identical: https://stackoverflow.com/questions/55826856/how-to-build-for-multiple-targets-with-webpack

@akanass
Copy link
Contributor

akanass commented Mar 30, 2021

@MasterKale The best solution is to stop using Webpack because this tool is old school and to have a better build system with multiple targets, the right tool to use is rollup.

I use it in my projects and even with the one that supports this library, I have no compilation problems and I output files in MJS for browsers supporting imports of ESM modules or even SystemJS compatible files with an import modules inside.

When we have multiple bundle, we need to have different entries point inside package.json to be used in other bundle system like main, browser, jsnext:main, module, ....

@akanass
Copy link
Contributor

akanass commented Mar 30, 2021

But once again, why compile this library in ES5 when the browsers that support WebAuthn all support ES2018 compilation.

Older browsers don't support WebAuthn so I don't see the point of all of this.

@mariusmarais
Copy link

mariusmarais commented Mar 30, 2021

@akanass I assume the underlying issue is that this non-ES5 module is blowing up the entire app. For the app WebAuthn is optional and only available in newer browsers. But the pure existence of the option is breaking the app in the older browsers. Since the app must work in older browsers because of reasons, the WebAuthn functionality cannot be used by anyone. (This is putting a lot of words in @Moumouls 's mouth, I hope the gist is correct.)

Objectively the code as written has a bug: it cannot possibly work as originally intended. To fix it, the detection code must be ES5 compatible, because it can +- never fail to detect, only blow up. The easiest thing to do is to make everything ES5, but ... reasons.

After successful detection, we know newer code can be loaded, but splitting on that level with dynamic imports is very complex and possibly out of scope for this module. The midway is to build both ES5 and newer versions, but that burden might be too high for this package, especially if it would require a build system switch (regardless of which is better or newer).

In short, the situation sucks. (At work I would have said "nuanced" 😄 ) So no-one is crazy, they just have different goals and @MasterKale wants to help without ruining his week.

@akanass
Copy link
Contributor

akanass commented Mar 30, 2021

@mariusmarais @MasterKale I totally agree with both of you and the best solution is too have multiple bundle in this library or just indicated to user who wants to use it inside their own project in ES5 that they have to transpile it.

You can see an example here how to transpile ES6 to ES5 with Webpack and babel https://medium.com/@sivaraj-v/basic-webpack-4-and-es5-to-es6-transpiler-using-babel-dc66e72c86c6

This library should not be in ES5 natively because people who are using dynamic import will be badly impacted and it should not be the case.

@akanass
Copy link
Contributor

akanass commented Mar 30, 2021

Here a build system with webpack and multiple targets output

@akanass
Copy link
Contributor

akanass commented Mar 30, 2021

With multiple bundles, the main entry of package.json will be the ES2018 version, like this all the people who have a good build system or who want to use dynamic import or module in browser will be able to do it and the browser entry of package.json will be the ES5 version, like this people who are using Webpack to build a browser version will have this version in their own build and they have to set the right option in their webpack.config to take it

@Moumouls
Copy link
Contributor

Moumouls commented Mar 30, 2021

@mariusmarais you are right the situation sucks.

So:
With ES5 9kb (without GZIP and tree shaking )
Without ES5 5kb (without GZIP and tree shaking )

So final diff is 4kb heavier...

Here a 4ko diff in 2021 looks like to me a good tradeoff to avoid over head/surprise for developers that use the lib and avoid to some developers to have their app (especially the login page) starting crashing on old browser since. Yes (sadly) in 2021 Legacy Edge and IE11 still seems to be used. The lib will work by default without over head or special attention of the developer.

In other hand, if developers want's to optimize their build there are also free to re compile the lib, dive into the compilation process to get rid off of the 4ko and add some additional configs/systems to load it correctly in their projects.

@akanass
Copy link
Contributor

akanass commented Mar 30, 2021

I don't agree on it because I have the ES2018 version in my project and I can build it in SYSTEMJS, COMMONJS, ESM, ES5 whatever and all is working without any problems because I am using the right tool to do it.

We have not to provide an old school version to fix problem or say to developers you have to rebuild the lib.

Developers should know how to build a ES2018 to ES5 without problem to include it in their own project and not the reverse.

All modern framework are using ES2018 as standard and provide polyfills if needed or just explain that you have to compile this version in ES5 if you need it.

ES5 as default is not the solution. Having multiple version in the build is one of the best or just make developers responsible and they have to do the job by their own

@akanass
Copy link
Contributor

akanass commented Mar 30, 2021

If you add a polyfill in your project to support ES2108 syntax all will work if you don't want to make the setup to compile in ES5 in your application.

Include the polyfill in your bundle process and all will be done

@Moumouls
Copy link
Contributor

So here since this issue is critical for my company use case and also for further Parse SDK implementation (Parse community). i opted out for a re published package under an alt org https://www.npmjs.com/search?q=%40simplewebauthn-alt.

I really don't like re publishing projects like this one, well constructed. 🙁 I can't let the apps crash on the old browser any longer...

@MasterKale i'll add some info on the README to indicate the reason of the re publish.

Note: the downgrade to ES5 works correctly, tested with browser stack and legacy edge.

@akanass
Copy link
Contributor

akanass commented Mar 31, 2021

@Moumouls you said that you're using Next.js application and the problem occurred with the spread operator but in the documentation of Next.js I can see they are supporting this operator with IE11 and they are giving some advanced configuration to allow babel to add more plugins and solve your problem I guess

@MasterKale
Copy link
Owner Author

So here since this issue is critical for my company use case and also for further Parse SDK implementation (Parse community). i opted out for a re published package under an alt org https://www.npmjs.com/search?q=%40simplewebauthn-alt.

I can't say I'm a fan of this approach - everything on all of those "-alt" listings links back to here. Can you not pull in @simplewebauthn/browser into your codebase temporarily while I work on an official solution to this problem?

@Moumouls
Copy link
Contributor

Moumouls commented Mar 31, 2021

@akanass Next.JS support IE11 for code compiled by Next (in our case React Components). Next ensure that compiled react components will work in IE11, Next does not guarantee/re compile dependencies (just perform some tree shaking). On all our packages that we use only simplewebauthn makes our app crashing.

@MasterKale sadly my team already use the re published package and we have also have an ETA scheduled for our customers tomorrow. We cannot remove the feature temporarily since our customers already use it and we can't keep an app crashing on login page on old browsers.

while I work on an official solution to this problem?

May be a temporary solution could be to change the compilation target to es5 while waiting the final solution (multi export mentioned by @akanass )

Then when the browser package will be patched, i will for sure delete the re published packages, and switch back to the original lib. 🙂

Here I just want to help, on our side we have Q/A teams waiting for an ETA, and the fastest way for us is just to re publish the package.

@akanass
Copy link
Contributor

akanass commented Mar 31, 2021

@Moumouls if they provide advanced settings to add babel loader it's for something and you can override the config to indicate which package you want to compile or not. Instead of exclude node_modules like all webpack.config does you can indicates that you want to compile simplewebauthn too.

When I am using rollup and I import browser functions, they are compiled inside my own files in the target that I want and I don't have problem even in old browser.

@Moumouls
Copy link
Contributor

thanks @akanass for the tips but on our side the release is already on the road and the fix works without over head or additional config. Now here i think it should be interesting to focus on how to support multi export here to switch back asap and avoid surprise for developers that do not dive into each package configs 🙂

@MasterKale
Copy link
Owner Author

The good news is I found some time to follow that article @akanass posted. I have a branch sitting on my laptop that builds the lib in a few different configurations (basically everything except CJS because it's not relevant in front end projects)! 🎉

The only blocker right now is finding a version injection plugin that will let me use multiline comment blocks to avoid issues with single line comments breaking some build configurations that have come up in the past...

@akanass: how would someone using a newer version of @simplewebauthn/browser, built similarly to that article you posted, "choose" the desired version of the bundled or unbundled files to use? And do .d.at files map to bundled and unbundled files too? So types would be available no matter which set of files a consuming project chose to reference?

@MasterKale
Copy link
Owner Author

@Moumouls I just pushed up a fix/issue-114-browser-multi-bundle branch here. Would you be willing to test out the output from the new @simplewebauthn/browser build process? You can npm run build:browser from the root folder to build it, and see the new build artifacts in packages/browser/dist/:

Screen Shot 2021-04-02 at 8 25 20 AM

In the meantime I'll be trying to find time and figure out the best way to test older browsers myself. I figured I'd put the work up here in case you were interested in being a bit of a guinea pig since this impacts you the most 😇

@MasterKale
Copy link
Owner Author

MasterKale commented Apr 2, 2021

And here's the new package.json - @akanass does this look right?

"main": "dist/esm/index.js",
"exports": {
  "import": "./dist/esm/index.js"
},
"types": "./dist/types/index.d.ts"

The way I see it the bundled UMD will be more suitable for linking to this package from something like unpkg, when you want to manually pull in a single JS file and host it the "old-fashioned" way, within a <script>.

@akanass
Copy link
Contributor

akanass commented Apr 2, 2021

@MasterKale I'm not sure it's OK with the package.json and even with the bundle.

I've checked your branch and the problem is that you only have an ESM version with ES2018 as target.

You have to build a version in ES5 and another one in ES2018.

For ES5 version, your tsconfig.json should have the properties:

{
  "compilerOptions": {
    "target": "es5",
    "module": "commonjs",
  },
  "lib": [
    "es5",
    "dom"
  ],
}

For ES2018 version, your tsconfig.json should have the properties:

{
  "compilerOptions": {
    "target": "es2018",
    "module": "es2020",
  },
  "lib": [
    "es2018",
    "dom"
  ],
}

And after in your package.json you should have:

{
  "main": "./dist/esm2018/index.js",
  "browser": "./dist/esm5/index.js",
  "types": "./dist/types/index.d.ts"
}

We want to have the latest version as default version that's why we put it in the main property and we create the browser entry that can be used by Webpack like explain here

The exports has not to be used here because we have only ESM build so they can be both imported by import or require keywords.

During the development, may import the version used for that in ES5, that in ES2018 or that in TS, what counts is how the elements are compiled inside the final project and I think the configuration like this will work.

I do not think the bundle versions are needed apart if you really want to provide the possibilities of being able to import directly into a script tag but I'm not a fan of what is done in the example because the bundle included far too much of something from babel.

It would be necessary to take either the sources already compiled in ES5, for the UMD version, and the ES2018 for the ESM version or use rollup to make the complete process of TS to JS then the bundle version but as is done here this is not at all optimal.

I think these bundle versions complicates the thing if you do not control rollup so the ES5 and ES2018 versions are largely sufficient in my opinion.

@MasterKale
Copy link
Owner Author

You have to build a version in ES5 and another one in ES2018.

My understanding is that Babel is responsible for transpiling down to ES5-compatible code based on the value of "targets" passed to @babel-env in .babelrc.js

@akanass
Copy link
Contributor

akanass commented Apr 2, 2021

@MasterKale you're right but this value is used only for the esmBundled env which uses rollup after.

But for your dist/esm you are not using it as we can see on your script here your are using the esmUnbundled env which means you are only compiling TS to JS with the default tsconfig.json so you will only have the ES2018 version and you can check the code if you still have or not the spread operator inside which is the reason of the browser crash.

If the code is transpiling down to ES5-compatible as you think and maybe it's the case, and if we only have this version, it's not good as well because we want to have both versions

@akanass
Copy link
Contributor

akanass commented Apr 2, 2021

Because we only want ES5 and ES2018 versions the easy way will be to use TSC to compile with one dedicated tsconfig file for both version instead of using babel

We don't need the bundled versions at all so make it simple and all will work.

And after if you really want to have the bundled version you can use the ES5 version as entry point for rollup and all will be perfect.

@akanass
Copy link
Contributor

akanass commented Apr 2, 2021

The example link I provided it was to explain the global scope but the easiest way to implement the thing is to use the Typescript compiler with two configuration files and the turn is played.

@MasterKale
Copy link
Owner Author

MasterKale commented Apr 2, 2021

In my mind we want "unbundled ESM targeting ES2018 (so that consumers can let their build tools handle polyfilling and transpiling down to ES5)", and "bundled UMD targeting ES5". So thinking about it some more I probably don't need to have Rollup produce anything ESM, just bundled UMD which will lean on Babel for targeting ES5.

Because we only want ES5 and ES2018 versions the easy way will be to use TSC to compile with one dedicated tsconfig file for both version instead of using babel

We don't need the bundled versions at all so make it simple and all will work.

I understand how you're proposing I simplify the build to just two executions of tsc, but that won't produce anything bundled, so it'd be impossible to use a service like unpkg to download a single JS file for easy copy/paste into an HTML file (which is currently supported, it's how I pull in Browser in example/)

@akanass
Copy link
Contributor

akanass commented Apr 2, 2021

@MasterKale it's not the case because @Moumouls is using the compiler of Next.js to bundle his application and inside the code you have the import of your library which means you have to provide an ES5 version too.

People won't only use script tag for the ES5 version. If they want to use your library inside their own script and they don't transpile it during their build process, an ES5 version has to be provided as well.

All browser librairies provide a bundle and a module version compatible or not with old browser. If you want to have the compatibilty, the module version has to be in both versions

@akanass
Copy link
Contributor

akanass commented Apr 2, 2021

@MasterKale

I understand how you're proposing I simplify the build to just two executions of tsc, but that won't produce anything bundled, so it'd be impossible to use a service like unpkg to download a single JS file for easy copy/paste into an HTML file (which is currently supported, it's how I pull in Browser in example/)

yes you can have the bundled version with rollup after compiling with TSC, you will use the output as entry point to build your bundled version and you won't have all boilerplate of babel because the entry point will be the ES5 version made by TSC

@akanass
Copy link
Contributor

akanass commented Apr 14, 2021

here is the webpack configuration with the overrides and we can see that the fields are taken into account in both the client and server parts because resole.mainFields contains the good values but that the SSR rendering does not care because the error persists during the build and it always wants to take the main field despite de facto overrides.

And even with alias it's not working, this framework is really rubbish because it does not take into account any of the compilation standards.

{
  externals: [ 'next' ],
  optimization: {
    noEmitOnErrors: false,
    checkWasmTypes: false,
    nodeEnv: false,
    splitChunks: {
      chunks: 'all',
      cacheGroups: [Object],
      maxInitialRequests: 25,
      minSize: 20000,
      hidePathInfo: true,
      minChunks: 1,
      maxAsyncRequests: 5,
      automaticNameDelimiter: '~',
      automaticNameMaxLength: 109,
      name: true
    },
    runtimeChunk: { name: 'webpack' },
    minimize: true,
    minimizer: [ [TerserPlugin], [CssMinimizerPlugin] ],
    removeAvailableModules: true,
    removeEmptyChunks: true,
    mergeDuplicateChunks: true,
    flagIncludedChunks: true,
    occurrenceOrder: true,
    sideEffects: true,
    providedExports: true,
    usedExports: true,
    concatenateModules: true,
    mangleWasmImports: false,
    namedModules: false,
    hashedModuleIds: false,
    namedChunks: false,
    portableRecords: false
  },
  context: '/Users/akanass/Programmation/debug/next-simplewebauthn',
  node: {
    setImmediate: false,
    console: false,
    process: true,
    global: true,
    Buffer: true,
    __filename: 'mock',
    __dirname: 'mock'
  },
  entry: {
    main: './node_modules/next/dist/client/next.js',
    polyfills: '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/client/polyfills.js',
    'pages/_app': [
      'next-client-pages-loader?page=%2F_app&absolutePagePath=private-next-pages%2F_app.js!',
      '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/client/router.js'
    ],
    'pages/index': 'next-client-pages-loader?page=%2F&absolutePagePath=private-next-pages%2Findex.tsx!',
    'pages/_error': 'next-client-pages-loader?page=%2F_error&absolutePagePath=next%2Fdist%2Fpages%2F_error!'
  },
  watchOptions: {
    aggregateTimeout: 5,
    ignored: [ '**/.git/**', '**/node_modules/**', '**/.next/**', '**/.#*' ]
  },
  output: {
    path: '/Users/akanass/Programmation/debug/next-simplewebauthn/.next',
    filename: 'static/chunks/[name]-[chunkhash].js',
    library: '_N_E',
    libraryTarget: 'assign',
    hotUpdateChunkFilename: 'static/webpack/[id].[hash].hot-update.js',
    hotUpdateMainFilename: 'static/webpack/[hash].hot-update.json',
    chunkFilename: 'static/chunks/[name].[contenthash].js',
    strictModuleExceptionHandling: true,
    crossOriginLoading: false,
    futureEmitAssets: true,
    webassemblyModuleFilename: 'static/wasm/[modulehash].wasm',
    hotUpdateFunction: 'webpackHotUpdate_N_E',
    jsonpFunction: 'webpackJsonp_N_E',
    chunkCallbackName: 'webpackChunk_N_E',
    globalObject: 'window',
    devtoolNamespace: '_N_E',
    pathinfo: false,
    sourceMapFilename: '[file].map[query]',
    jsonpScriptType: false,
    chunkLoadTimeout: 120000,
    hashFunction: 'md4',
    hashDigest: 'hex',
    hashDigestLength: 20,
    devtoolLineToLine: false
  },
  performance: false,
  resolve: {
    extensions: [
      '.mjs',  '.js',
      '.tsx',  '.ts',
      '.jsx',  '.json',
      '.wasm'
    ],
    modules: [ 'node_modules' ],
    alias: {
      next: '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next',
      'private-next-pages': '/Users/akanass/Programmation/debug/next-simplewebauthn/pages',
      'private-dot-next': '/Users/akanass/Programmation/debug/next-simplewebauthn/.next',
      'unfetch$': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/polyfills/fetch/index.js',
      'isomorphic-unfetch$': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/polyfills/fetch/index.js',
      'whatwg-fetch$': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/polyfills/fetch/whatwg-fetch.js',
      'object-assign$': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/polyfills/object-assign.js',
      'object.assign/auto': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/polyfills/object.assign/auto.js',
      'object.assign/implementation': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/polyfills/object.assign/implementation.js',
      'object.assign$': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/polyfills/object.assign/index.js',
      'object.assign/polyfill': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/polyfills/object.assign/polyfill.js',
      'object.assign/shim': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/polyfills/object.assign/shim.js',
      url: '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/native-url/dist/index.js',
      '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/next-server/lib/router/utils/resolve-rewrites.js': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/next-server/lib/router/utils/resolve-rewrites-noop.js'
    },
    mainFields: [ 'browser', 'module', 'main' ],
    plugins: [ [Object] ],
    unsafeCache: true,
    mainFiles: [ 'index' ],
    aliasFields: [ 'browser' ],
    cacheWithContext: true
  },
  resolveLoader: {
    alias: {
      'emit-file-loader': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/webpack/loaders/emit-file-loader',
      'error-loader': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/webpack/loaders/error-loader',
      'next-babel-loader': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/webpack/loaders/next-babel-loader',
      'next-client-pages-loader': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/webpack/loaders/next-client-pages-loader',
      'next-serverless-loader': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/webpack/loaders/next-serverless-loader',
      'noop-loader': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/webpack/loaders/noop-loader',
      'next-plugin-loader': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/webpack/loaders/next-plugin-loader',
      'next-style-loader': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/webpack/loaders/next-style-loader'
    },
    modules: [ 'node_modules' ],
    plugins: [ [Object] ],
    unsafeCache: true,
    mainFields: [ 'loader', 'main' ],
    extensions: [ '.js', '.json' ],
    mainFiles: [ 'index' ],
    roots: [ '/Users/akanass/Programmation/debug/next-simplewebauthn' ],
    cacheWithContext: true
  },
  module: {
    rules: [ [Object], [Object] ],
    strictExportPresence: true,
    unknownContextRequest: '.',
    unknownContextRegExp: false,
    unknownContextRecursive: true,
    unknownContextCritical: true,
    exprContextRequest: '.',
    exprContextRegExp: false,
    exprContextRecursive: true,
    exprContextCritical: true,
    wrappedContextRegExp: /.*/,
    wrappedContextRecursive: true,
    wrappedContextCritical: false,
    strictThisContextOnImports: false,
    unsafeCache: false,
    defaultRules: [ [Object], [Object], [Object], [Object] ]
  },
  plugins: [
    ChunkNamesPlugin {},
    DefinePlugin { definitions: [Object] },
    ReactLoadablePlugin { filename: 'react-loadable-manifest.json' },
    DropClientPage { ampPages: Set(0) {} },
    HashedModuleIdsPlugin { options: [Object] },
    IgnorePlugin {
      options: [Object],
      checkIgnore: [Function: bound checkIgnore]
    },
    BuildManifestPlugin {
      buildId: 'u9j4IpfONe0_UCaqG6u61',
      rewrites: [Object]
    },
    ProfilingPlugin { compiler: [Compiler] },
    WellKnownErrorsPlugin {},
    NextMiniCssExtractPlugin {
      options: [Object],
      __next_css_remove: true
    }
  ],
  mode: 'production',
  name: 'client',
  target: 'web',
  bail: true,
  devtool: false,
  cache: false,
  infrastructureLogging: { level: 'info', debug: false }
}
{
  externals: [ [Function (anonymous)] ],
  optimization: {
    noEmitOnErrors: false,
    checkWasmTypes: false,
    nodeEnv: false,
    splitChunks: false,
    runtimeChunk: undefined,
    minimize: false,
    minimizer: [ [TerserPlugin], [CssMinimizerPlugin] ],
    removeAvailableModules: true,
    removeEmptyChunks: true,
    mergeDuplicateChunks: true,
    flagIncludedChunks: true,
    occurrenceOrder: true,
    sideEffects: true,
    providedExports: true,
    usedExports: true,
    concatenateModules: true,
    mangleWasmImports: false,
    namedModules: false,
    hashedModuleIds: false,
    namedChunks: false,
    portableRecords: false
  },
  context: '/Users/akanass/Programmation/debug/next-simplewebauthn',
  node: {
    setImmediate: false,
    console: false,
    process: true,
    global: true,
    Buffer: true,
    __filename: 'mock',
    __dirname: 'mock'
  },
  entry: {
    'pages/_app': [ 'private-next-pages/_app.js' ],
    'pages/api/hello': [ 'private-next-pages/api/hello.js' ],
    'pages/index': [ 'private-next-pages/index.tsx' ],
    'pages/_error': [ 'next/dist/pages/_error' ],
    'pages/_document': [ 'next/dist/pages/_document' ],
    'init-server.js': 'next-plugin-loader?middleware=on-init-server!',
    'on-error-server.js': 'next-plugin-loader?middleware=on-error-server!'
  },
  watchOptions: {
    aggregateTimeout: 5,
    ignored: [ '**/.git/**', '**/node_modules/**', '**/.next/**', '**/.#*' ]
  },
  output: {
    path: '/Users/akanass/Programmation/debug/next-simplewebauthn/.next/server',
    filename: '[name].js',
    library: '',
    libraryTarget: 'commonjs2',
    hotUpdateChunkFilename: 'static/webpack/[id].[hash].hot-update.js',
    hotUpdateMainFilename: 'static/webpack/[hash].hot-update.json',
    chunkFilename: '[name].[contenthash].js',
    strictModuleExceptionHandling: true,
    crossOriginLoading: false,
    futureEmitAssets: true,
    webassemblyModuleFilename: 'static/wasm/[modulehash].wasm',
    hotUpdateFunction: 'webpackHotUpdate',
    jsonpFunction: 'webpackJsonp',
    chunkCallbackName: 'webpackChunk',
    globalObject: 'global',
    devtoolNamespace: '',
    pathinfo: false,
    sourceMapFilename: '[file].map[query]',
    jsonpScriptType: false,
    chunkLoadTimeout: 120000,
    hashFunction: 'md4',
    hashDigest: 'hex',
    hashDigestLength: 20,
    devtoolLineToLine: false
  },
  performance: false,
  resolve: {
    extensions: [
      '.js',   '.mjs',
      '.tsx',  '.ts',
      '.jsx',  '.json',
      '.wasm'
    ],
    modules: [ 'node_modules' ],
    alias: {
      next: '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next',
      'private-next-pages': '/Users/akanass/Programmation/debug/next-simplewebauthn/pages',
      'private-dot-next': '/Users/akanass/Programmation/debug/next-simplewebauthn/.next',
      '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/next-server/lib/router/utils/resolve-rewrites.js': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/next-server/lib/router/utils/resolve-rewrites-noop.js',
      '@simplewebauthn/browser': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/@simplewebauthn/browser/dist/es5'
    },
    mainFields: [ 'browser', 'module', 'main' ],
    plugins: [ [Object] ],
    unsafeCache: true,
    mainFiles: [ 'index' ],
    aliasFields: [],
    cacheWithContext: true
  },
  resolveLoader: {
    alias: {
      'emit-file-loader': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/webpack/loaders/emit-file-loader',
      'error-loader': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/webpack/loaders/error-loader',
      'next-babel-loader': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/webpack/loaders/next-babel-loader',
      'next-client-pages-loader': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/webpack/loaders/next-client-pages-loader',
      'next-serverless-loader': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/webpack/loaders/next-serverless-loader',
      'noop-loader': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/webpack/loaders/noop-loader',
      'next-plugin-loader': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/webpack/loaders/next-plugin-loader',
      'next-style-loader': '/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/webpack/loaders/next-style-loader'
    },
    modules: [ 'node_modules' ],
    plugins: [ [Object] ],
    unsafeCache: true,
    mainFields: [ 'loader', 'main' ],
    extensions: [ '.js', '.json' ],
    mainFiles: [ 'index' ],
    roots: [ '/Users/akanass/Programmation/debug/next-simplewebauthn' ],
    cacheWithContext: true
  },
  module: {
    rules: [ [Object], [Object] ],
    strictExportPresence: true,
    unknownContextRequest: '.',
    unknownContextRegExp: false,
    unknownContextRecursive: true,
    unknownContextCritical: true,
    exprContextRequest: '.',
    exprContextRegExp: false,
    exprContextRecursive: true,
    exprContextCritical: true,
    wrappedContextRegExp: /.*/,
    wrappedContextRecursive: true,
    wrappedContextCritical: false,
    strictThisContextOnImports: false,
    unsafeCache: false,
    defaultRules: [ [Object], [Object], [Object], [Object] ]
  },
  plugins: [
    ChunkNamesPlugin {},
    DefinePlugin { definitions: [Object] },
    HashedModuleIdsPlugin { options: [Object] },
    IgnorePlugin {
      options: [Object],
      checkIgnore: [Function: bound checkIgnore]
    },
    PagesManifestPlugin { serverless: false, dev: false },
    NextJsSsrImportPlugin { options: [Object] },
    NextJsSsrImportPlugin {},
    ProfilingPlugin { compiler: [Compiler] },
    WellKnownErrorsPlugin {}
  ],
  mode: 'production',
  name: 'server',
  target: 'node',
  bail: true,
  devtool: false,
  cache: false,
  infrastructureLogging: { level: 'info', debug: false }
}

@akanass
Copy link
Contributor

akanass commented Apr 14, 2021

@Moumouls in material-ui they are only compiling in ES5 that's why it's working but with this library we're compiling in multiple version and Next.js in SSR are only using main field of package.json because it is coded with the feet and it only uses the libraries at runtime and not at compilation so the use of the webpack override is useless with the SSR

I'm trying something else and if it's not working I know how to fix it but I don't like this solution

@akanass
Copy link
Contributor

akanass commented Apr 14, 2021

@Moumouls I switch the compilation for @swan/browser to ES5 and I still have issue because window isn't define in node so it can't work with SSR

$ next build
info  - Using webpack 4. Reason: future.webpack5 option not enabled https://nextjs.org/docs/messages/webpack5
info  - Checking validity of types  
info  - Creating an optimized production build  
info  - Compiled successfully
info  - Collecting page data  
[  ==] info  - Generating static pages (0/3)
Error occurred prerendering page "/". Read more: https://nextjs.org/docs/messages/prerender-error
ReferenceError: window is not defined
    at supportsWebauthn (/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/@simplewebauthn/browser/dist/es5/index.js:72:5)
    at Home (/Users/akanass/Programmation/debug/next-simplewebauthn/.next/server/pages/index.js:118:118)
    at d (/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/react-dom/cjs/react-dom-server.node.production.min.js:33:498)
    at bb (/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/react-dom/cjs/react-dom-server.node.production.min.js:36:16)
    at a.b.render (/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/react-dom/cjs/react-dom-server.node.production.min.js:42:43)
    at a.b.read (/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/react-dom/cjs/react-dom-server.node.production.min.js:41:83)
    at exports.renderToString (/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/react-dom/cjs/react-dom-server.node.production.min.js:52:138)
    at Object.renderPage (/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/next-server/server/render.js:54:854)
    at Function.getInitialProps (/Users/akanass/Programmation/debug/next-simplewebauthn/.next/server/pages/_document.js:719:19)
    at loadGetInitialProps (/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/next-server/lib/utils.js:5:101)
info  - Generating static pages (3/3)

> Build error occurred
Error: Export encountered errors on following paths:
        /
    at /Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/export/index.js:31:1106
    at async Span.traceAsyncFn (/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/telemetry/trace/trace.js:5:584)
    at async /Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/index.js:43:49
    at async Span.traceAsyncFn (/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/telemetry/trace/trace.js:5:584)
    at async /Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/build/index.js:25:1475
    at async Span.traceAsyncFn (/Users/akanass/Programmation/debug/next-simplewebauthn/node_modules/next/dist/telemetry/trace/trace.js:5:584)
error Command failed with exit code 1.

So now, I have to find the way for this one and again as we can see, we can't use single browser functionalities on this framework and it's not normal because now it's not a problem with the build, even if it was not the case in the past, but with their runtime

@Moumouls
Copy link
Contributor

Moumouls commented Apr 14, 2021

yes here it's normal, since the component is fully rendered via SSR. In normal production use case, support function should be only ran on browser (a non sense of running it in SSR). Some business logic could be implemented on this function to support SSR but it's non sense for webauthn package.

So if you succeed the compilation step, then window issue is super easy to fix and understand for a developer.

Then for better DX a custom error could be raised in support function when window do not exists to inform developers that this function should not run into SSR env.

@akanass
Copy link
Contributor

akanass commented Apr 14, 2021

@Moumouls @MasterKale I found the solution and all is working now without touching the library compilation and only dealing with Next.js

I use the advanced feature of Next.js to not use @swan/browser in SSR and like this all is working like a charm.

I have created a new component file ../components/supportWebauthn.tsx like this:

import { supportsWebauthn } from "@simplewebauthn/browser";

export function SupportsWebauthn() {
    return <h1>Support webauthn: {supportsWebauthn() + ''}</h1>;
}

And then using the advanced feature in index.tsx like this:

import dynamic from 'next/dynamic'

const DynamicComponentWithNoSSR = dynamic(
    () => import('../components/supportWebauthn').then(mod => mod.SupportsWebauthn),
    { ssr: false }
)

export default function Home() {
    return <DynamicComponentWithNoSSR/>;
}

AND THAT'S IT - ALL IS WORKING NOW => COMPILATION AND RENDERING

@akanass
Copy link
Contributor

akanass commented Apr 14, 2021

@Moumouls can you test it and validate it but all is fine like this on my side.

@Moumouls
Copy link
Contributor

i'll try this tomorrow ! thanks @akanass for you research 🙂

If this solution workds, it looks like super easy to use in Next.js project 👍

@akanass
Copy link
Contributor

akanass commented Apr 14, 2021

@Moumouls it'll work :D this is the official solution of Next.js and I did it with your own project so I can't do more and as you said, the WebAuthn detection should only be done in client side so like this it's the case.

I am looking forward for your feedback on it.

@Moumouls
Copy link
Contributor

Yes, i'll keep you posted tomorrow !

@MasterKale
Copy link
Owner Author

I remember when I first encountered SSR quirks a couple years ago messing around with Gatsby: their SSR workflow often broke if you used anything involving browser globals in your JavaScript. The fix always involved refactoring that particular bit of JS to include an environment check to make sure the code was running in a browser via whatever Gatsby flag was available to track that. If the flag indicated SSR, then the code didn't execute and Gatsby built the page.

I'm glad to see SSR is still terribly obtuse and requires non-obvious solutions like @akanass', it keeps us all employed 😂

@Moumouls
Copy link
Contributor

Moumouls commented Apr 15, 2021

First of all @akanass solution works 🎉

But i noticed 3 things that makes this library not so easy to use in real project usage:

  • Point 1: Each component that use @simplewebatuhn pkg need to use the dynamic import system
  • Point 2: Dynamic import only support React components but not Hooks, so we can fall into loading a whole frame via dynamic import
  • Point 3: Last one; Dynamic import as it name suggest, is dynamic... so noticable display latency occur and make the UI pop.

Point 1: could be fixed with some refactoring but need a special attention each time the lib is used
Point 2: It's a Next issue but a workaround is possible with point 3
Point 3: could be fixed with loading system but to avoid UI pop effect, a skeleton system is needed, but since dynamic import could also resolve fast, a flicker effect could happen on the app

Finally, yes the solution works, but the optimization/building/error global cost is too high for just 110 lines compared to a simple switch to es5 target by default on the browser package and 4 more kb.

I think here we spent lot of effort and time.

So on my side sadly i'll stick to the alt browser es5 version.

May be a special package browser/es5 with TS types could be a good tradeoff to avoid all of these issues and make the library working out of the box without any overhead waiting old browsers to be completely replaced may be in 2-3 years.

@akanass
Copy link
Contributor

akanass commented Apr 15, 2021

@Moumouls I am really surprised by the comments because we realize more and more the limitation of the framework and the technology used because by putting an official solution proposed by Next.js, it still does not suit.

You had build issues because the system does not allow to be overridden in SSR and the solution is that which allows to use a library only compatible with browsers and this is the case here because the global variables window and navigator are called.

This library will never be able to work on the server side so it will never be possible to have SSR with it and that is why the solution proposed by Next.js is made like this.

Even if there was the basic compilation in ES5, the SSR would not work because I did the test yesterday and all the functionalities should be framed by an environment test to know if we are in the browser or not.

This means that the code will necessarily have to be changed with a version dedicated to the browser and one to the SSR.

Then you tell us that the problem is now with React which does not support Hooks with dynamic imports and again it is a specific problem of the framework and the technology because the limitation is on this side since they offer a solution for the framework but which does not satisfy you.

As you say, a lot of time and effort has gone into helping you and we found a solution that uses compilation standards but is not accepted by your framework.

We found the official solution to use a library only compatible with browsers, which is the case here, and this solution is official and it does not work yet.

I find that it's a lot of things that become more and more specific to your case where you encourage an earlier version by default just to meet your need, because what you use is not up to standards and offers solutions that do not suit you, and therefore that all other users will have to do specific builds to have the recent version and use the modules in compatible browsers.

In addition to say that you prefer to use your own forked version instead I find that not nice because I think that this version is not maintained, it does not offer the expertise of @MasterKale and the help of the community.

In addition, as already said a version in ES5 will not solve the problem of SSR because all the tests will have to be done for the use of the functionalities which are specific to the browser so please give us valid arguments on this point and how you are going to proceed to fix it and what is the end use and the visual result because it intrigues me a lot to see this.

Then I will make the modification and I will propose ES5 as the default version and I will explain in the documentation how to have ES2018 for the others but it is going to be a major change for everyone because it makes a rollback just because of specifics and that displeases me enormously.

@akanass
Copy link
Contributor

akanass commented Apr 15, 2021

The new version of the bundle with ES5 by default allow the build of Next.js application as you can see here:

$ next build
info  - Using webpack 4. Reason: future.webpack5 option not enabled https://nextjs.org/docs/messages/webpack5
info  - Checking validity of types  
info  - Creating an optimized production build  
info  - Compiled successfully
info  - Collecting page data  
info  - Generating static pages (3/3)
info  - Finalizing page optimization  

Page                                                           Size     First Load JS
┌ ○ /                                                          3.67 kB        67.2 kB
├   /_app                                                      0 B            63.5 kB
├ ○ /404                                                       3.46 kB          67 kB
└ λ /api/hello                                                 0 B            63.5 kB
+ First Load JS shared by all                                  63.5 kB
  ├ chunks/f6078781a05fe1bcb0902d23dbbb2662c8d200b3.63e895.js  13.4 kB
  ├ chunks/framework.4b1bec.js                                 41.8 kB
  ├ chunks/main.13e85a.js                                      7.12 kB
  ├ chunks/pages/_app.3be4f3.js                                529 B
  ├ chunks/webpack.50bee0.js                                   751 B
  └ css/381f5b9c92d1673af027.css                               203 B

λ  (Server)  server-side renders at runtime (uses getInitialProps or getServerSideProps)
○  (Static)  automatically rendered as static HTML (uses no initial props)
●  (SSG)     automatically generated as static HTML + JSON (uses getStaticProps)
   (ISR)     incremental static regeneration (uses revalidate in getStaticProps)

✨  Done in 8.11s.

I only have one component with a test of the global window variable to allow SSR:

import { supportsWebauthn } from "@simplewebauthn/browser";

export default function Home() {
    return <h1>Support webauthn: {typeof window !== 'undefined' ? supportsWebauthn() + '' : '-'}</h1>;
}

We see the change of the dash by the value returned by the supportsWebauthn() function and this leads to a small visual adaptation because we are moving from the SSR version to the browser version so a solution will have to be put in place to have a better rendering but this is out of context from this library.

I also further optimised the build in ES5 which is now the default value of the library with a better final weight.

My branch and my PR will be made available soon.

@MasterKale
Copy link
Owner Author

@Moumouls Thank you for sharing your experiences using SimpleWebAuthn with a SSR framework. I've primarily focused on using @simplewebauthn/browser in pure-front-end contexts, like Create-React-App projects, but I acknowledge that your use case is just as valid.

Reading down your list of pain points, none of what you're experiencing is new - it's all SSR framework woes, you'd have these same issues with Gatsby, or whatever other framework comes out next. Things like "loading a whole frame via dynamic import" and "noticeable display latency occur and make the UI pop" require architectural considerations as you build out your application to account for these SSR-based "nuances". If you can build out your application without using globals like window then generally it's an okay time. But the moment you want to use any front-end-only API in an SSR page you're going to have to deal with issues like this. Whether or not @simplewebauthn/browser targets ES5 as the default, you may one day pull in another package that that causes similar issues and you'll have to work with whatever tools the SSR framework gets you.

All that said, let's focus on your issue some more. I finally pulled down that reproduction repo you provided and yep, it's broken 😂. I did some research, though, and I believe you're a victim of this super-old Next.JS issue (2017!!):

vercel/next.js#706

The root cause seems to be that Next.JS stopped/never transpiles anything in node_modules/, so of course browser's new ESM build is never transpiled to CommonJS for Next's SSR functionality to be able to import.

A fix was proposed in that issue but never seems to have been implemented. Within, though, I found a reference to next-transpile-modules and managed to get your repo working with it!

Screen Shot 2021-04-15 at 7 35 14 AM

After installing that package I created a next.config.js and set up '@simplewebauthn/browser' to be transpiled:

const transpileModules = require('next-transpile-modules');

const withTM = transpileModules([
  '@simplewebauthn/browser',
]);

module.exports = withTM();

Then, after some SSR-related refactor (that would be required in any SSR framework, not just Next) to index.tsx the page loaded fine:

import { supportsWebauthn } from "@simplewebauthn/browser";

const isServer = () => typeof window === 'undefined';

export default function Home() {
  let supports = false;
  if (!isServer()) {
    supports = supportsWebauthn();
  }

  return <h1>{`Support webauthn: ${supports}`}</h1>;
}

@MasterKale
Copy link
Owner Author

MasterKale commented Apr 15, 2021

FYI that isServer trick above is what I used to do in these situations, back when I used Gatsby, to only invoke code that calls browser-only APIs when I can reasonably assume the code is running in a browser. I think it's worth remembering as it'll surely come in handy in the future 😉

@akanass
Copy link
Contributor

akanass commented Apr 15, 2021

@MasterKale the PR is done and all is working on my side with both versions.

You can create a new branche and put the PR inside to test it if you want before merging in master

@akanass
Copy link
Contributor

akanass commented Apr 15, 2021

@MasterKale I did a PR and an example with the same trick as you as explain in my previous comment

And you don't need a transpiled config with this new bundle if we keep ES5 as default version and the new ES5 is even better now so we have to use it.

If you don't want to have ES5 as default I have just to rollback the package.json and README but for the ES5 build we have to use it like this which is a better one than the previous one.

@Moumouls
Copy link
Contributor

Moumouls commented Apr 15, 2021

I'm back, yeah here it seems that Next.js sucks. We all agree that this lib is fine and the main fields are also ok.

Of course in our app we already have the window check to avoid calling some core browser apis on the server for the webauthn package.

i think your PR @akanass will be the best solution with default ES5, it will avoid i think so many build/rendering issue in the futur 🙂

Feel free to mention me once it's merged with may be a new NPM release. I'll be happy to perform tests on our app at the office 👍 But if es5 is the default entry, no surprise here it'll work perfectly out of the box.

@akanass
Copy link
Contributor

akanass commented Apr 15, 2021

@Moumouls I agree with you even if for modern browsers usages we have to do a dedicated build but anyway the new ES5 version is better now so all this pain is not for anything.

@MasterKale you have the PR #119 for you and all is ready - you have just to change the base branch with your own one like this you can adapt the documentation and test my new builds before merging in master

All is better now and despite the framework which is not adapted in the standard systems, this allowed to improve even more the final build so it is a bad for a good

@MasterKale
Copy link
Owner Author

PR #119 has been merged, but I'm going to get one more fix into Browser before I cut another release. I'll follow up here when there's a new version ready for @Moumouls' consideration :)

@MasterKale
Copy link
Owner Author

I finally got a new release out - this should now be fixed in @simplewebauthn/browser@3.0.0! 🎉

See https://github.com/MasterKale/SimpleWebAuthn/releases/tag/v3.0.0 for more info.

@Moumouls
Copy link
Contributor

i'll try this asap 💯

@MasterKale
Copy link
Owner Author

@Moumouls don't leave us hanging! 😂

@Moumouls
Copy link
Contributor

Yes sorry, I've a task planned on this for Monday or Tuesday haha 😊

@Moumouls
Copy link
Contributor

Moumouls commented May 5, 2021

@Moumouls
Copy link
Contributor

Moumouls commented May 5, 2021

alt packages should be now removed from npm.

@MasterKale
Copy link
Owner Author

Alright then, time to close out this ticket. Great job everyone 😄

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

No branches or pull requests

4 participants