Skip to content

[remix] Fix config file dynamic import#9249

Merged
kodiakhq[bot] merged 5 commits intomainfrom
fix-remix-build
Jan 18, 2023
Merged

[remix] Fix config file dynamic import#9249
kodiakhq[bot] merged 5 commits intomainfrom
fix-remix-build

Conversation

@TooTallNate
Copy link
Copy Markdown
Member

@TooTallNate TooTallNate commented Jan 18, 2023

Follow-up to #8793, which breaks dynamic import of the config file, causing some issues for remix users: https://github.com/orgs/vercel/discussions/1282

The underlying error is MODULE_NOT_FOUND.

import() should work with file:// URIs, however, since it's using typescript, the import gets compiled to require(), which does not support file:// protocol scheme.

Supersedes #9248.

They get run by the integration tests instead, and currently Jest
segfaults with `eval('import(...)')` so we can't really unit test
anymore :(
@TooTallNate TooTallNate changed the title Fix remix build [remix] Fix config file dynamic import Jan 18, 2023
"@types/node": "14.18.33",
"@vercel/build-utils": "5.9.0",
"typescript": "4.6.4"
"typescript": "4.9.4"
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thought this was necessary at first. This isn't the case, but doesn't hurt either.

"build": "node build.js",
"test-integration-once": "pnpm test test/test.js",
"test": "jest --env node --verbose --bail --runInBand",
"test-unit": "pnpm test test/build.test.ts"
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The unit tests were deleted since they test basically the same as the integration tests. However, await import() breaks Jest currently, so they needed to be removed.

const remixConfigModule = await import(
pathToFileURL(remixConfigFile).href
);
const remixConfigModule = await eval('import(remixConfigFile)');
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

eval is required to prevent TypeScript from compiling the import into require.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would be surprised if this works since I didn’t think eval works for ESM

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A better solution is to update tsconfig.json with module: nodenext

@TooTallNate TooTallNate marked this pull request as ready for review January 18, 2023 10:16
Copy link
Copy Markdown
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Can you add a test?

@TooTallNate
Copy link
Copy Markdown
Member Author

The existing integration test should already be testing for this. I don't really understand how it was passing before, but possibly just a caching issue 😕

@kodiakhq kodiakhq bot merged commit 02ff265 into main Jan 18, 2023
@kodiakhq kodiakhq bot deleted the fix-remix-build branch January 18, 2023 11:24
styfle added a commit that referenced this pull request Jan 18, 2023
kodiakhq bot pushed a commit that referenced this pull request Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: remix semver: patch PR contains bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants