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 global crypto to @astrojs/webapi #6981

Merged
merged 6 commits into from May 26, 2023

Conversation

andremralves
Copy link
Contributor

@andremralves andremralves commented May 4, 2023

Changes

fixes #6870

  • Add the global crypto to be polyfilled in the webapi package.

I need some opinions to finish this issue

Currently the global crypto will not be polyfilled even if I add it to the webapi package (using node version<=18).

That is because it's already partially polyfilled by @astrojs/compiler in the global scope here: https://github.com/withastro/compiler/blob/bedced877522e3a14ec9416b592a2c185b8fd0d5/packages/compiler/node/wasm_exec.ts#L25-L33

The code above just polyfills the getRandomValue method and It runs before the applyPolyfill function is called in Astro's core. So the polyfill for crypto is skiped as it already exists (but with only one method).

I thought about some alternatives to solve that:

  1. Polyfill the entire crypto interface in the @astrorjs/compiler package.
  2. Call the polyfill from webapi before the compiler's files are imported.
  3. Create a HACK in the webapi package to polyfill just the missing properties from crypto.
  4. Remove the global polyfill from @astrorjs/compiler (I tested and nothing broke, but I don't know if can affect in something else).

Testing

...

Docs

No docs necessary

@changeset-bot
Copy link

changeset-bot bot commented May 4, 2023

🦋 Changeset detected

Latest commit: bac6255

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Princesseuh
Copy link
Member

While withastro/compiler#790 fixes the issue in dev, I would assume that in prod SSR, this is still an issue, right? I'm personally for this PR.

@andremralves andremralves marked this pull request as ready for review May 17, 2023 15:53
@andremralves
Copy link
Contributor Author

You are right @Princesseuh. Good catch! I tested locally and in prod SSR we still can't use crypto globally.

image

I also noticed that I get a type error when I import webcrypto because the @types/node package used by webapi is still on version v14.18.21, maybe it should be updated as v14 is no longer supported and Astro uses version >=16.12.

@Princesseuh Princesseuh merged commit bf63f61 into withastro:main May 26, 2023
3 of 5 checks passed
@astrobot-houston astrobot-houston mentioned this pull request May 26, 2023
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.

crypto global is only partially polyfilled
3 participants