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

Fix image integration crash on Netlify Functions due to import.meta.url #5888

Merged
merged 2 commits into from
Jan 19, 2023

Conversation

Princesseuh
Copy link
Member

@Princesseuh Princesseuh commented Jan 18, 2023

Changes

Netlify Functions are still on CJS, however they support ESM files by running them through esbuild, this is great, but esbuild doesn't polyfill import.meta.url so our function just immediately crash.

This PR add a helper that either return import.meta.url if supported, or an URL using __dirname so it works both directly in ESM and hazardly transpiled CJS

Fix #5774

Note

There's apparently a way to get Netlify functions to run in pure ESM mode already, using a per-function config file (an undocumented feature) and a (also undocumented) flag, but I could never get it to work despite multiple attempts 🤷‍♀️

Netlify is shipping pure ESM functions natively with no hidden flag soon, so this is really just a temporary workaround that we'll be able to remove in the future (or keep if it affects other hosts, who knows)

Testing

Tested manually, this issue only shows up when running directly on Netlify

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Jan 18, 2023

🦋 Changeset detected

Latest commit: 9e6be6d

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

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Jan 18, 2023
*/
export function getModuleURL(url: string | undefined): string {
if (!url) {
return pathToFileURL(__dirname).toString();
Copy link
Member

@bluwy bluwy Jan 18, 2023

Choose a reason for hiding this comment

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

The __dirname refers relative to this utils file. Would it work correctly if it's used by the consumer file later on?

I see some doing

scriptDirectory = dirname(getModuleURL(import.meta.url)) + '/'

which might have a different value depending on whether import.meta.url is supported.

Copy link
Member Author

@Princesseuh Princesseuh Jan 19, 2023

Choose a reason for hiding this comment

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

It's admittedly not ideal, but it does work because the __dirname call is only used in SSR where everything gets bundled 😅

If you have a better solution, I'll definitely take! Got lost in between the __dirname, __filename and import.meta.url while working on this so really I settled on the solution that actually did work, but I'm not picky 😵‍💫

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. Didn't know this was bundled so it would always have the same value.

This trick sounds good to me then, though I wonder if it should be __filename instead since that reflects import.meta.url. If you've tested with __dirname and it works then I'll be fine by it too 👍

@Princesseuh Princesseuh merged commit 35b26f3 into main Jan 19, 2023
@Princesseuh Princesseuh deleted the fix/netlify-image branch January 19, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Installing Image component with Netlify adaptor crashes site in prod.
2 participants