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

collab :) #1

Merged
merged 4 commits into from
May 18, 2024
Merged

collab :) #1

merged 4 commits into from
May 18, 2024

Conversation

devjiwonchoi
Copy link

No description provided.

@devjiwonchoi devjiwonchoi marked this pull request as ready for review May 18, 2024 17:41
@devjiwonchoi
Copy link
Author

@Arindam200 Hi, feel free to ask question if any doubt on changes!

@devjiwonchoi
Copy link
Author

devjiwonchoi commented May 18, 2024

Or else we can squash the changes :)

@devjiwonchoi
Copy link
Author

cc @mertcanaltin

@@ -396,7 +402,8 @@ async function run(): Promise<void> {
program.app = Boolean(appRouter)
}
}
if (!process.argv.includes('--turbo') && !process.argv.includes('--no-turbo')) {

if (!program.turbo) {

Choose a reason for hiding this comment

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

Suggested change
if (!program.turbo) {
if (!process.argv.includes('--turbo')) {

Copy link
Author

Choose a reason for hiding this comment

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

It's a program.turbo since it uses commandjs, where .option passes value as turbo there

Copy link
Author

Choose a reason for hiding this comment

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

e.g. --empty to .option passes program.empty value in Commander

Choose a reason for hiding this comment

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

I understand, thank you very much. I suggested it because this was the general usage 👍

Copy link
Author

Choose a reason for hiding this comment

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

Awesome, thank you for reviewing as well 😌

@mertcanaltin
Copy link

@devjiwonchoi
Copy link
Author

devjiwonchoi commented May 18, 2024

Since we're not adding another template, IMHO we don't need add a template type?
This Issue is mainly reading the arg --turbo and add it to package.json script.dev as next dev --turbo

Template types seem to targeting for templates.

@Arindam200
Copy link
Owner

Since we're not adding another template, IMHO we don't need add a template type? This Issue is mainly reading the arg --turbo and add it to package.json script.dev as next dev --turbo

I also think so.

@Arindam200
Copy link
Owner

Since we're not adding another template, IMHO we don't need add a template type? This Issue is mainly reading the arg --turbo and add it to package.json script.dev as next dev --turbo
Template types seem to targeting for templates.

Ah yes there is no turbo templating I guess we don't need to add it, we can do it in a separate pr ❤️ 🚀

Yes. So, we can merge this PR

@devjiwonchoi
Copy link
Author

Cool, let's ship it!!

@Arindam200
Copy link
Owner

Merging this!

@Arindam200 Arindam200 merged commit 8d0c0da into canary May 18, 2024
@devjiwonchoi devjiwonchoi deleted the add-turbo-test branch May 18, 2024 18:02
@mertcanaltin
Copy link

I agree with you, maybe we can also offer another pr 🚀

Arindam200 pushed a commit that referenced this pull request Aug 16, 2024
…etPrefix (vercel#68694)

This PR fixes two issues with the use of `assetPrefix`:

#1: vercel#64710
`assetPrefix` needs to be handled in `dev`, `deploy`, and `start`. In
the current approach, only `dev` and `start` were handled, but a quirk
of the implementation caused rewrites for non-asset paths to not be able
to be used in `afterFiles` rewrites.

#2: When deploying Next.js (such as on Vercel), you need to add your own
`beforeFiles` rewrite for `/${assetPrefix}/_next/...` requests or
otherwise they would 404.

This PR creates an automatically added `rewrite` to `beforeFiles` that
handles the case for `dev`, `start`, and `deploy`, removes the existing
logic in `filesystem.ts`, and adds more tests to check the behavior.
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.

3 participants