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

[PFX-637] Support create App Router #777

Merged
merged 9 commits into from
Jun 10, 2024
Merged

[PFX-637] Support create App Router #777

merged 9 commits into from
Jun 10, 2024

Conversation

jpina1-godaddy
Copy link
Contributor

@jpina1-godaddy jpina1-godaddy commented Jun 5, 2024

Summary

Added prompt to use App Router with the Gasket Nextjs plugin.

It does not currently work with Redux. This needs to be addressed in a separate ticket. We also need to port any logic from _app and _document files in a separate ticket.

I also removed the generated test from the cypress plugin. This test was conflicting with the generated cypress test in the nextjs plugin.

Changelog

@gasket/plugin-cypress

  • Removed generated index test

@gasket/plugin-nextjs

  • Added App Router prompt
  • Modified generated files to add App Router generated files

Test Plan

  • Unit tests
  • Tested locally

@jpina1-godaddy jpina1-godaddy changed the title feat: added app router prompt to next plugin [PFX-637] Support create App Router Jun 6, 2024
@jpina1-godaddy jpina1-godaddy marked this pull request as ready for review June 6, 2024 01:35
@jpina1-godaddy jpina1-godaddy requested a review from a team as a code owner June 6, 2024 01:35
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update with the latest from my PR that fixed Page Router rendering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


export default function RootLayout({ children }) {
return (
<html lang='en'>
Copy link
Contributor

@jpage-godaddy jpage-godaddy Jun 7, 2024

Choose a reason for hiding this comment

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

We shouldn't presume lang='en'

Copy link
Contributor Author

@jpina1-godaddy jpina1-godaddy Jun 7, 2024

Choose a reason for hiding this comment

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

Eslint looks for a lang value here <html> elements must have the lang prop jsx-a11y/html-has-lang.

What would be a good default for this?

Would en be safe for the boilerplate code?

]);

newContext.useAppRouter = useAppRouter;
newContext.nextServerType = 'defaultServer';
Copy link
Contributor

@jpage-godaddy jpage-godaddy Jun 7, 2024

Choose a reason for hiding this comment

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

Wouldn't this line get skipped if useAppRouter was already in the context? What would happen if nextServerType was not initialized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a check outside of the useAppRouter prompt

  if (newContext.useAppRouter) {
    newContext.nextServerType = 'defaultServer';
  }

@jpina1-godaddy jpina1-godaddy merged commit 9b056be into v7 Jun 10, 2024
1 check passed
@jpina1-godaddy jpina1-godaddy deleted the pfx-637 branch June 10, 2024 20:45
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.

4 participants