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

chore: expose globalThis.crypto when not available #48941

Merged
merged 7 commits into from
Apr 28, 2023

Conversation

balazsorban44
Copy link
Member

@balazsorban44 balazsorban44 commented Apr 28, 2023

What?

Exposing globalThis.crypto, based on Node.js' WebCrypto API

Why?

Similar to fetch, crypto is a popular API that is currently not available on globalThis in all active Node.js versions yet.

This can help library authors to create runtime-agnostic packages.

How?

Node.js already has the WebCrypto API that can be imported, we just expose it on globalThis in Node.js versions where this is not available.

Closes NEXT-1063

Slack thread

)
}

export const runtime = 'nodejs'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set explicitly so the test will work indefinitely in the future. Note: "edge" already exposes globalThis.crypto for us.

)
}

export const runtime = 'nodejs'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ijjk
Copy link
Member

ijjk commented Apr 28, 2023

Failing test suites

Commit: f70efc0

pnpm testheadless test/e2e/app-dir/crypto-globally-available/crypto-globally-available.test.ts

  • Web Crypto API is available globally > should be available in Server Components
  • Web Crypto API is available globally > should be available in Route Handlers
Expand output

● Web Crypto API is available globally › should be available in Server Components

expect(received).toBe(expected) // Object.is equality

Expected: "crypto is available"
Received: "crypto is not available"

  10 |     it('should be available in Server Components', async () => {
  11 |       const browser = await next.browser('/')
> 12 |       expect(await browser.elementByCss('p').text()).toBe('crypto is available')
     |                                                      ^
  13 |     })
  14 |
  15 |     // In case you need to test the response object

  at Object.<anonymous> (e2e/app-dir/crypto-globally-available/crypto-globally-available.test.ts:12:54)

● Web Crypto API is available globally › should be available in Route Handlers

expect(received).toContain(expected) // indexOf

Expected substring: "crypto is available"
Received string:    "crypto is not available"

  17 |       const res = await next.fetch('/handler')
  18 |       const html = await res.text()
> 19 |       expect(html).toContain('crypto is available')
     |                    ^
  20 |     })
  21 |   }
  22 | )

  at Object.<anonymous> (e2e/app-dir/crypto-globally-available/crypto-globally-available.test.ts:19:20)

Read more about building and testing Next.js in contributing.md.

@balazsorban44 balazsorban44 marked this pull request as draft April 28, 2023 08:39
@balazsorban44 balazsorban44 marked this pull request as ready for review April 28, 2023 08:51
@balazsorban44 balazsorban44 requested a review from a team as a code owner April 28, 2023 08:51
@@ -249,7 +249,7 @@
"@types/react-dom": "18.0.11"
},
"engines": {
"node": ">=16"
"node": ">=16.8.0"
Copy link
Member Author

@balazsorban44 balazsorban44 Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that webcrypto was actually added back in 15.0.0 https://nodejs.org/dist/latest-v18.x/docs/api/crypto.html#cryptowebcrypto

But we still want to bump this version since undici (what we use in App Router) requires 16.8.0+.

Let me know if this should be a different PR.

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks Balazs 💯

@timneutkens timneutkens merged commit 88a033f into canary Apr 28, 2023
@timneutkens timneutkens deleted the chore/polyfill-crypto branch April 28, 2023 09:18
kodiakhq bot pushed a commit that referenced this pull request Apr 28, 2023
We bumped `undici` fetch which has a minimum version of 16.8.0 so we need to make sure `next` and `create-next-app` also have the same minimum version.

Since 14.x reaches End-of-Life on [2023-04-30](https://github.com/nodejs/Release), we can drop support for 14 in the next release.

See also:

- Related to #48870
- Related #48941
@t3dotgg
Copy link
Contributor

t3dotgg commented Apr 29, 2023

Wait this unbreaks a ton of stuff for me thank you so much

@g12i g12i mentioned this pull request May 5, 2023
timneutkens added a commit that referenced this pull request May 9, 2023
### What?

Allow overwriting the `global.crypto` property when polyfilling it.

### Why?

#48941 introduced `global.crypto` polyfill. The problem is that if some
library (e.g.
[xksuid](https://github.com/ValeriaVG/xksuid/blob/main/src/index.node.mjs))
tries to do the same thing, it breaks as `global.crypto` is defined as
non-writable[^1]. Arguably libraries should check for `global.crypto`
presence before overwriting it BUT I think polyfill should match the
actual implementation[^2].



### How?

Make `global.crypto` `enumerable` and `configurable`, as well as define
`set` implementation[^3].

[^1]:
![image](https://user-images.githubusercontent.com/7079786/236440322-7bcf1b18-8fcc-4bb9-b9b4-0f2eb032f5ba.png)

[^2]:
![image](https://user-images.githubusercontent.com/7079786/236437260-d3abdb0c-134f-4c9d-aab8-de7bf4d7c831.png)

[^3]:
![image](https://user-images.githubusercontent.com/7079786/236440393-1c469035-a9f1-4fbe-9ce7-c0308e980510.png)

---------

Co-authored-by: Tim Neutkens <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants