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

Node 18 support #206

Merged
merged 7 commits into from
Aug 25, 2024
Merged

Node 18 support #206

merged 7 commits into from
Aug 25, 2024

Conversation

versecafe
Copy link
Collaborator

Fixes #189

Just references crypto with an import instead of assuming in name space

+ import * as crypto from 'crypto';

  export function randomid(byteSize = 16) {
    const bytes = crypto.getRandomValues(new Uint8Array(byteSize));
    return base32hexnopad.encode(bytes).toLowerCase();
  }

@nichochar
Copy link
Contributor

Indeed my cursory exploration of node18 compat only brought up this issue. I will do a bit of QA to make sure most of the functionality works correctly, but otherwise LGTM.

We're not heavily prioritizing earlier versions FYI, since we might want to use some recent features like replacing tsx with vanilla node with --experimental-strip-types.

However, if it's easy in the meantime, I'm all for compatibility.

@versecafe
Copy link
Collaborator Author

--experimental-strip-types is probably something I'd recommend avoiding until it's more stable unless you allow users on bun to run the TS proper, and find a solution for people on Deno, and it could be removed at any time

@@ -1,5 +1,6 @@
import { base32hexnopad } from '@scure/base';
import type { CodeLanguageType } from './types/cells.js';
import * as crypto from 'crypto';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is used on the client in addition to the Node backend. What happens in that environment? Is vite smart enough to remove this from the compiled code, or does it try to do some sort of polyfill (e.g. the way webpack used to with crypto-browserify)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should technically work Vite doesn't do a polyfil but ignores the import which should then cause it to end up using the global window crypto instance, or polyfills can be added if for some reason it's not but it does seem to be working

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not want us to use polyfills for something that exists natively. So assuming this drops the import then I'm stoked to get this working in node 18.

srcbook/README.md Show resolved Hide resolved
Copy link
Contributor

@benjreinhart benjreinhart left a comment

Choose a reason for hiding this comment

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

I couldn't find anything in a quick scan of vite's docs. My only concern would be if vite were to include a polyfill in our build that we don't need since I don't want to blow up the client side code for no reason (like webpack would)

I'm going to assume the above isn't the case and approve, but if you have any references in the docs you can provide that'd be reassuring

@versecafe
Copy link
Collaborator Author

@benjreinhart it will give a warning of

[plugin:vite:resolve] [plugin vite:resolve] Module "crypto" has been externalized for browser compatibility, imported by "/Users/veronica/Projects/srcbook/packages/shared/dist/src/utils.js". See https://vitejs.dev/guide/troubleshooting.html#module-externalized-for-browser-compatibility for more details.

but this just means it'll fail if you try to call the node version of crypto on the browser since no pollyfill is used, but the isBrowser check means that on a browser it will always end up using globalThis.crypto which is built in

@benjreinhart benjreinhart merged commit a16434a into srcbookdev:main Aug 25, 2024
3 checks passed
@versecafe versecafe deleted the node-18-support branch August 25, 2024 21:55
@nichochar
Copy link
Contributor

Let's go! Thanks @versecafe 🔥 🚀

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.

support nodejs v18
3 participants