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

Add crypto to Polyfills improving Blueprint compatibility for Node #1000

Merged
merged 4 commits into from Feb 5, 2024

Conversation

sejas
Copy link
Collaborator

@sejas sejas commented Feb 5, 2024

What is this PR doing?

It imports the native library crypto and makes it globally available in the runtime.

What problem is it solving?

When using blueprints that install plugins or themes, it generates a random folder using crypto`. That library is available in the browsers by default, but for node we need to import it.

Note that it do not install any dependency since Crypto is already built in NodeJS, but it's not imported by default.
I decided to use node:crypto which has webcrypto and will be a better match. It's available sine Node v15.

The other alternative is normal crypto https://nodejs.org/docs/latest-v14.x/api/crypto.html, which also has crypto.randomUUID. It's available since Node v14.

How is the problem addressed?

It adds a new Polyfill for crypto.

Testing Instructions

  1. Comment the first line import './crypto'; on packages/php-wasm/node-polyfills/src/lib/crypto.spec.ts
  2. Run npx nx test php-wasm-node-polyfills
  3. Observe the tests fail
  4. Uncomment first line import './crypto'; on packages/php-wasm/node-polyfills/src/lib/crypto.spec.ts
  5. Run npx nx test php-wasm-node-polyfills
  6. Observe the tests pass

@adamziel
Copy link
Collaborator

adamziel commented Feb 5, 2024

To fix the e2e failure you might need to add node:crypto to external in packages/php-wasm/node/vite.config.ts

@sejas
Copy link
Collaborator Author

sejas commented Feb 5, 2024

@adamziel , thanks for the pointer ! 🙏

Updated! 927a6b3

@adamziel
Copy link
Collaborator

adamziel commented Feb 5, 2024

Hm maybe I was wrong. It seems to dislike the node: prefix. What if you import from just crypto instead of node:crypto?

@adamziel adamziel merged commit a4d1bd6 into trunk Feb 5, 2024
5 checks passed
@adamziel adamziel deleted the add/crypto-php-wasm-node-polyfills branch February 5, 2024 22:05
@adamziel
Copy link
Collaborator

adamziel commented Feb 5, 2024

Thank you @sejas!

@sejas
Copy link
Collaborator Author

sejas commented Feb 8, 2024

@adamziel , this fix seems to not be working when the JS is in NPM package.

The "transpiled" JS is like:
node_modules/@wp-playground/blueprints/index.js

typeof crypto > "u" && import("./__vite-browser-external-2447137e.js").then((e) => {
  global.crypto = e;
});

Where ./__vite-browser-external-2447137e.js:

const e = {};
export {
  e as default
};

It shouldn't import from ./__vite-browser-external-2447137e.js , but instead from crypto. 🤔 .
Do you have in mind any possible solution?

Should we pivot and use a custom plain function, instead of crypto.randomUUID. I've seen it's just used a couple of times to create a temporal file. Nothing that needs to be super secure. A simple new Date().getTime() should be enough to create a unique file.

Should we use a third party library?

Should be handled on node clients directly?

@adamziel
Copy link
Collaborator

adamziel commented Feb 8, 2024

Yeah let’s pivot to a custom randomString() function, we could move the one that’s already implemented to generate WP secrets@php-wasm/utils

@adamziel
Copy link
Collaborator

adamziel commented Feb 8, 2024

export function randomString(length: number) {

@sejas
Copy link
Collaborator Author

sejas commented Feb 8, 2024

I'll prepare a PR 🤞

adamziel pushed a commit that referenced this pull request Feb 8, 2024
…1016)

- Remove the crypto polyfill introduced on
#1000
- Move the `randomString` from `remote` package to `@php-wasm/util`
- Created a new custom function `randomFilename`
- Replace the usage of `crypto.randomUUID` with `randomFilename()`

## What problem is it solving?

`crypto` is a library that is available, but needs to be imported on
node apps.

## How is the problem addressed?

It replaces the function that generates a random value with our custom
library

## Testing Instructions

- CI tests pass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants