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

feat(nextjs): Add Next 13 to peer dependencies and integration tests #6042

Merged
merged 3 commits into from
Oct 27, 2022
Merged

feat(nextjs): Add Next 13 to peer dependencies and integration tests #6042

merged 3 commits into from
Oct 27, 2022

Conversation

koenpunt
Copy link
Contributor

@koenpunt koenpunt commented Oct 25, 2022

This adds Next.js 13 as a supported peer dependency.

Had to do a bit of a hack for the legacyBehavior prop for <Link /> components, since their behavior changed in this new version of Next.js. Eventually a more future proof solution has to be found, once they no longer support the legacyBehavior prop.

Fixes #6046


# Next.js v13 requires React 18.2.0
if [ "$NEXTJS_VERSION" -eq "13" ]; then
npm i --save [email protected] [email protected]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this maybe use yarn instead?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I tried running the full test suite with react 18.2.0 and it works fine, even for Next 10, so rather than doing it here, let's just change package.json and use that version for everything.

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 figured that you want to make sure it also still works with older versions of react, hence the package update. If you think that's not necessary I'll update the default version to 18.

Copy link
Member

Choose a reason for hiding this comment

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

IMO let's stick with the older versions, especially to get this in so we can release. We can re-evaluate bumping the overall version later on.

@bintzandt
Copy link

bintzandt commented Oct 26, 2022

Maybe it is a better idea to use the new convention for <Link>?

Especially easy since they have a codemod available that you can simply run.

Sorry if this seems like a random comment; I just wondered upon this PR and I thought that you might not know about the available codemod.

@koenpunt
Copy link
Contributor Author

Maybe it is a better idea to use the new convention for <Link>?

If I do that, the tests for older versions fail.

Especially easy since they have a codemod available that you can simply run.

I like the codemod, maybe I just run that in the test instead, and not modify the source, but lets wait for a maintainer to give some feedback.

Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, @koenpunt!

Comment on lines 10 to 12
// Defined as any so it can also be used in versions of Next.js that do not support the legacyBehavior prop.
const linkProps: any = { legacyBehavior: true };

Copy link
Member

Choose a reason for hiding this comment

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

Just ran the tests against Next 13 without the legacyBehavior prop and it all seemed to work. Just to confirm, I also ran it with a page like this:

const HealthyPage = (): JSX.Element => (
  <Link href="/alsoHealthy">
    <a>
      <a>
        <a>
          <a>
            <a id="alsoHealthy">AlsoHealthy</a>
          </a>
        </a>
      </a>
    </a>
  </Link>
);

It seems the nested <a> tags don't pose a problem, so for simplicity, let's not worry about adding linkProps everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me the integration tests fail when not adding the legacyBehavior prop.

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've now pushed the changes without the legacyBehavior prop commit, to see what it will do on CI.

Copy link
Contributor Author

@koenpunt koenpunt Oct 27, 2022

Choose a reason for hiding this comment

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

Maybe it was just a fluke on my machine, because it seems to be OK on CI now 👍.

packages/nextjs/test/run-integration-tests.sh Outdated Show resolved Hide resolved

# Next.js v13 requires React 18.2.0
if [ "$NEXTJS_VERSION" -eq "13" ]; then
npm i --save [email protected] [email protected]
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I tried running the full test suite with react 18.2.0 and it works fine, even for Next 10, so rather than doing it here, let's just change package.json and use that version for everything.

@lobsterkatie lobsterkatie changed the title Add next 13 as supported peer dependency feat(nextjs): Add Next 13 to peer dependencies and integration tests Oct 26, 2022
@lobsterkatie lobsterkatie self-assigned this Oct 27, 2022
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Thanks a lot @koenpunt - turned on CI so let's see what it has to say, but if that goes green let's merge and cut a new release!

@AbhiPrasad AbhiPrasad enabled auto-merge (squash) October 27, 2022 11:08
@AbhiPrasad AbhiPrasad merged commit 8880fa1 into getsentry:master Oct 27, 2022
@koenpunt koenpunt deleted the next13 branch October 27, 2022 11:12
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.

Upgrade peerDependencies of @sentry/nextjs to next 13
4 participants