-
-
Notifications
You must be signed in to change notification settings - Fork 52
fix(netlify): replace global.crypto
with crypto
#163
Conversation
🦋 Changeset detectedLatest commit: 14e01c2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
cc @Skn0tt, wdyt about this change? |
In Astro, it does polyfill the global crypto object here. Does it fail for you? Or are you loading the netlify integration manually outside of Astro? |
Looks fine to me! I don't think the global crypto polyfill will work, because this is about usage within the build, not within the runtime. And we only put that polyfill in place for the runtime. |
The code I linked is used to apply the polyfill in build time. https://github.com/withastro/astro/blob/2dbb6a3e082bd1e33cc0de2f10417a012ae89243/packages/astro/src/core/build/index.ts#L68 |
Interesting, didn't know that! So why is |
So I've just tested in a docker ( As Node 18 is still maintenance for another year, I still thing this is a valid change. But perhaps this is the wrong change to fix this problem. I will highlight though, the other packages in this repo import crypto instead of using the global version |
@OiYouYeahYou does it only happen with the |
@lilnasy If I remember rightly, this was a blocker on |
I can only reproduce it with ![]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me!
The PR to astro will enable the use of crypto
and File
in the config file for new versions. This PR can help with compatibility with older versions.
Could you add a patch changeset with pnpm -w exec changeset
? It will show up in CHANGELOG.md.
crypto
package instead of global.crypto
global.crypto
undefined
global.crypto
undefinedglobal.crypto
with crypto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to docs, but as mentioned in the comment, would love to be specific about version numbers if it's easy, but not a deal breaker!
(Especially if by chance it now works for some older versions, but not all of them, would be nice for people to know whether or not it should work for their version after this fix.)
.changeset/young-ravens-hammer.md
Outdated
"@astrojs/netlify": patch | ||
--- | ||
|
||
Fixes an issue where using `global.crypto` failed for older Astro versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we give a version number here to be more specific? Something like:
failed for Astro vx.x and earlier
(Assuming this fix works for all earlier Astro versions. Otherwise, specifying the range would be a nice addition.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lilnasy do you know which versions this applies to? I guess: <4.4.1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was fixed in 4.4.1.
Changes
This is a drive by change because
randomUUID
is not on the global version ofcrypto
on my node version (v18.18.2
). I'm uncertain if this breaks any compatibility for others and do not have pnpm to run and commands.Testing
Not testing done because this is a trivial change and don't have pnpm
Docs
Trivial change, probably should be noted in changelog