[create-cloudflare] next.js with ts fix#7854
Conversation
🦋 Changeset detectedLatest commit: 20992d5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
dario-piotrowicz (asking maintainer to run E2E tests separately) |
|
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243642388/npm-package-wrangler-7854You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7854/npm-package-wrangler-7854Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243642388/npm-package-wrangler-7854 dev path/to/script.jsAdditional artifacts:cloudflare-workers-bindings-extension: wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243642388/npm-package-cloudflare-workers-bindings-extension-7854 -O ./cloudflare-workers-bindings-extension.0.0.0-va059128b2.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-va059128b2.vsixcreate-cloudflare: npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243642388/npm-package-create-cloudflare-7854 --no-auto-update@cloudflare/kv-asset-handler: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243642388/npm-package-cloudflare-kv-asset-handler-7854miniflare: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243642388/npm-package-miniflare-7854@cloudflare/pages-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243642388/npm-package-cloudflare-pages-shared-7854@cloudflare/unenv-preset: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243642388/npm-package-cloudflare-unenv-preset-7854@cloudflare/vite-plugin: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243642388/npm-package-cloudflare-vite-plugin-7854@cloudflare/vitest-pool-workers: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243642388/npm-package-cloudflare-vitest-pool-workers-7854@cloudflare/workers-editor-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243642388/npm-package-cloudflare-workers-editor-shared-7854@cloudflare/workers-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243642388/npm-package-cloudflare-workers-shared-7854@cloudflare/workflows-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243642388/npm-package-cloudflare-workflows-shared-7854Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
| // use bindings during local development (when running the application with \`next dev\`). | ||
| // For more information see: | ||
| // https://github.com/cloudflare/next-on-pages/blob/main/internal-packages/next-dev/README.md | ||
| if (process.env.NODE_ENV === 'development') { |
There was a problem hiding this comment.
@dario-piotrowicz do we need (process.env.NODE_ENV === 'development') here? (or could it be inside setupDevPlatform ?
(The code here is only moved from l42 so not directly related to the PR)
There was a problem hiding this comment.
This check is totally unnecessary, moving it inside setupDevPlatform wouldn't actually accomplish anything
it's here to try to make it extra clear to the developer that this function is only needed in development, in reality this only has any effect during development anyways
I'm happy to get rid of it if you want 👍
There was a problem hiding this comment.
vicb
left a comment
There was a problem hiding this comment.
I took a quick look here but I don't really think the changes are optimal, there should be no reason to need both a TS and JS version of updateNextXConfig.
One thing that look weird is that we do not pass --js or --ts to create-next-app
JS version only prepends some code to existing config while TS version removes the existing one completely and creates a new hardcoded one from scratch. I can make JS version also behave like TS version but in case of a possible config change by next.js team, both versions will be affected instead of only the TS version.
We don't need that, user decides that during next-cli prompt |
Thanks for the clarification here.
Could you expand on why the TS config would need to be fully replaced? (maybe add the generated TS config + explanation to the PR description). Thanks! |
No problem
I've just edited the description. Please also check the conversation at the linked issue. |
I think we should drop the @dario-piotrowicz what do you think? |
yeah I forgot about the |
|
I've reverted all of my previous work.
Removed
Removed the if and improved the comment to emphasize that. I hope it is OK now. |
Co-authored-by: Victor Berchet <victor@suumit.com>
vicb
left a comment
There was a problem hiding this comment.
LGTM, thanks for the PR and the updates!
I triggered the e2e
Co-authored-by: Victor Berchet <victor@suumit.com>
|
Thanks for your directions and your patience. It was fun to work on this. |
|
@beratbayram it looks like the formatting is wrong, could you please run |
done |
|
Congratulations @beratbayram, the maintainer of this repository has issued you a holobyte! Here it is: https://holopin.io/holobyte/cm6z97fao45380cl72xu6wqq0 This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account. |
Fixes #7665.
WHAT: It changes the output of next.js with TS template.
WHY: Basic templating is broken.
HOW: Existing customers are not affected. New customers will have a better experience.
Related bug only appears when nextjs is used with TS because C3 generates a top-level
awaitstatement inside a.tsfile which is illegal. So this PR doesn't change how JS version is generated but creates a newnext.config.mjsfile with new hardcoded content for TS version specifically.Only TS version is hardcoded to minimize the impact of possible config file change by the next.js team but we can discuss to do that to JS version as well for simplicity.
Other possible solutions are discussed at the linked issue but to summarise:
.mtsfile type can't be used because next config can't be a.mtsfile(1).I've run tests and two fails as follows but I don't believe they are related to my changes.