crouchcd / pkce-challenge

Generate or verify a Proof Key for Code Exchange (PKCE) challenge pair
MIT License
72 stars 13 forks source link

Top-level await breaks esbuild build #24

Closed mdarocha closed 10 months ago

mdarocha commented 1 year ago

I'm using pkce-challenge with esbuild to build a web bundle, and since v4 I can't build the app, due to usage of top-level await in this library.

✘ [ERROR] Top-level await is currently not supported with the "iife" output format

    node_modules/pkce-challenge/dist/index.js:3:5:
      3 │     (await import("node:crypto")).webcrypto; // Node.js 16 non-REPL
        ╵      ~~~~~

Is there a way to change this import to avoid usage of this await?

crouchcd commented 1 year ago

@mdarocha Sorry about the troubles there. pkce-challenge@v4 does a couple of things differently,

Because of these changes, using await import() is the only option for supporting both Web Browser and Node.js (v16) environments.

See the relevant source code for how Web Crypto API is imported https://github.com/crouchcd/pkce-challenge/blob/0b5a1a35b83bfb973ca492a2d8df9fe1b163d066/src/index.ts#L3-L6

esbuild support of top-level await was released with limited support https://github.com/evanw/esbuild/releases/tag/v0.10.0. Unfortunately, iife is not supported.

If v3 was working for you before I suggest to continue using that until top-level await is fully supported in esbuild. The business logic has not changed between v3 and v4, only how the module is imported/called.

1hko commented 1 year ago

I had the same issue using 4.x and switched to 3.x but encountered a different problem. I used pnpm patch to fix the package.json and the build worked.

diff --git a/package.json b/package.json
index 8d03007c97148e101a56e9164de81b376cc5b1d0..1814e97d89b44bebbd2c3edd5c4b40f1ef05b3d4 100644
--- a/package.json
+++ b/package.json
@@ -1,11 +1,15 @@
 {
   "name": "pkce-challenge",
+  "type": "module",
   "version": "3.1.0",
   "description": "Generate or verify a Proof Key for Code Exchange (PKCE) challenge pair",
   "source": "src/index.ts",
-  "main": "dist/main.js",
-  "module": "dist/module.js",
-  "types": "dist/index.d.ts",
+  "exports": {
+    ".": {
+      "types": "./dist/index.d.ts",
+      "import": "./dist/module.js",
+      "default": "./dist/main.js"
+    }
+  },
   "files": [
     "dist/"
   ],

Then I noticed the crypto-js dependency in 3.x creates a bloated build. Then I found @aaronpk/pkce-vanilla-js.

mdarocha commented 1 year ago

It would fix this problem if you provided a "browser" field in package.json, pointing to a file that does not attempt to import node-specific dependencies. You could also use static imports, instead of resolving dependencies at runtime.

mdarocha commented 1 year ago

It would fix this problem if you provided a "browser" field in package.json, pointing to a file that does not attempt to import node-specific dependencies. You could also use static imports, instead of resolving dependencies at runtime.

Implemented in https://github.com/crouchcd/pkce-challenge/pull/25

seanmcquaid commented 11 months ago

Hello everyone! Any updates on the progress on this? I am trying to use this package for my browser based app but have ran into this error.

saschanaz commented 11 months ago

Perhaps it's time to simply remove Node.js 16 support? When the change happened Node 16 LTS was alive and expected to live long enough, but things changed: https://nodejs.org/en/blog/announcements/nodejs16-eol

We are moving the End-of-Life date of Node.js 16 by seven months to coincide with the end of support of OpenSSL 1.1.1 on September 11th, 2023.

We are past the end of support date, so we should just remove it and simplify things.

Edit: Sigh, it turns out Node.js 18 still doesn't have globalThis.crypto unless on REPL. Only Node.js 19+ have it.

mdarocha commented 10 months ago

Since my PR with the fix got merged, can we close this issue? Or is it waiting for a release or something else?

crouchcd commented 10 months ago

@mdarocha I am waiting for release before closing. Will work on this later tonight.

crouchcd commented 10 months ago

Fixed in #4.1.0. Please let me know if you have further issues after this release.