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

Taking care of next/babel preset #21104

Merged

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Feb 15, 2023

Resolves #20087
Resolves #21027

What I did:

Moved next/babel preset into codebase to be able to make changes, because it seems that next/babel will not be maintained anymore in favour of swc/Turbopack. Also:

  • Fixed inconsistent propTypes handling between dev + build modes in NextJs.
  • Removed precompiled @babel/typescript-preset package from next and use the latest one.

Background:

The Next.js babel plugin next/babel behaves differently in development and production modes. It removes React propTypes in production, which is bad because Storybook relies on them to correctly infer the right argTypes. I opened a discussion here at @vercel/nextjs to be able to conditionally still output the proptypes even for a production build. For almost two months nobody replied to this. I assume, that they will not work on any babel-related stuff, because the future at Next.js is Turbopack.

Essentially the plugin-transform-react-remove-prop-types plugin applied here should not be applied on production.

I tried different approaches:

  1. My first approach was flattening all plugins in next/babel preset and removing the superfluous plugin-transform-react-remove-prop-types plugin if it is present. The issue is that as soon as I want to look up the babel configuration, I cannot identify a plugin by a specific ID or name, because of how the plugins are set up in next/babel (direct require call).

  2. The final approach I have decided on, and which was discussed with @shilman, was to actually copy next/babel and all of its dependencies to modify that one line and reference the modified next/babel preset instead. I really don't like this approach, but I can't think of another solution.

Therefore actually, only the changes in code/frameworks/nextjs/src/preset.ts matter. The other files are more or less copied over with some slight type adjustments.

How to test

The chromatic changes introduced here
https://www.chromatic.com/build?appId=634ff0454a50407deedde524&number=205
should be reverted by
https://www.chromatic.com/build?appId=634ff0d0ec053b270775979d&number=2459

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@valentinpalkovic valentinpalkovic self-assigned this Feb 15, 2023
@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-proptypes-for-nextjs-in-production branch from fc47549 to 66bbc44 Compare February 15, 2023 15:29
@valentinpalkovic valentinpalkovic added the ci:daily Run the CI jobs that normally run in the daily job. label Feb 15, 2023
@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-proptypes-for-nextjs-in-production branch from 66bbc44 to 3f14c24 Compare February 23, 2023 10:37
@valentinpalkovic valentinpalkovic changed the title Fix proptypes for Next.js in production Taking care of next/babel preset Feb 23, 2023
Copy link
Contributor

@kasperpeulen kasperpeulen left a comment

Choose a reason for hiding this comment

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

Great!

@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-proptypes-for-nextjs-in-production branch from c00e1dc to 66cdcd7 Compare February 23, 2023 12:42
@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-proptypes-for-nextjs-in-production branch from 66cdcd7 to 3cc2d7f Compare February 27, 2023 08:51
@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-proptypes-for-nextjs-in-production branch from 3cc2d7f to 84b8f68 Compare February 27, 2023 09:13
@valentinpalkovic valentinpalkovic merged commit b22c487 into next Feb 27, 2023
@valentinpalkovic valentinpalkovic deleted the valentin/fix-proptypes-for-nextjs-in-production branch February 27, 2023 13:02
@shilman shilman added the nextjs label Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ci:daily Run the CI jobs that normally run in the daily job. nextjs
Projects
None yet
3 participants