search

pastelsky / bundlephobia

🏋️ Find out the cost of adding a new frontend dependency to your project
https://bundlephobia.com
MIT License
9.02k stars 223 forks source link

migrate to next 13 #704

Closed darasus closed 2 years ago

darasus commented 2 years ago

What?

Why? Breaking changes while migrating from Nextjs 11 to 13.

Tested?

pastelsky commented 2 years ago

Thanks for submitting the update.

move all sass imports to stylesheets folder and moving imports to _app

Why has this been done? I'd rather have the styles for components co-located with their code. If newer versions of Next don't allow co-located style imports, I'd still keep them collocated and import them all inside a index.scss in stylesheets so later, these can be refactored into CSS-in-JS / CSS modules easily.

darasus commented 2 years ago

@pastelsky fair point. What I did in the last commit I reverted component scss file locations except for page level sass, this can't stay in pages directory.

pastelsky commented 2 years ago

Why have all components been moved from their organised page directories to a flat folder? This makes things quite unmaintainable. What was next complaining about here?

darasus commented 2 years ago

Why have all components been moved from their organised page directories to a flat folder? This makes things quite unmaintainable. What was next complaining about here?

pastelsky commented 2 years ago

don't see the need to keep another set of components inside pages directory

The top level components directory contains re-useable components, while the ones inside the page's directory are specifically for use in the page.

Let's please make the minimum amount of changes required to upgrade to Next 13. Changing the entire code hierarchy in a single PR is un-necessary, hard to review and quite confusing — these need to be discussed before a change is considered.

darasus commented 2 years ago

don't see the need to keep another set of components inside pages directory

The top level components directory contains re-useable components, while the ones inside the page's directory are specifically for use in the page.

Let's please make the minimum amount of changes required to upgrade to Next 13. Changing the entire code hierarchy in a single PR is un-necessary, hard to review and quite confusing — these need to be discussed before a change is considered.

Agreed, should have been discussed first.

darasus commented 2 years ago

@pastelsky I believe I considered all your comments above, please have a look. Please pay attention that we use webpack5 now and your override is not present anymore. What kind of patch did you have there?

pastelsky commented 2 years ago

I still think there are quite a few un-related changes made in this PR — kindly only keep the minimum change required to upgrade to Next 13, and undo everything else — raise a separate issue / PR if needed for those.

darasus commented 2 years ago

@pastelsky could you give an example of what change is not relevant to next 13 migration?

pastelsky commented 2 years ago

@darasus I've added comments for the same — could you check the "pending" comments from the latest review?

darasus commented 2 years ago

@pastelsky I believe all pending comments should be resolved now, basically removed eslint package that was installed by next's postinstall

pastelsky commented 2 years ago

@darasus There are open comments that haven't been responded too :) I'm listing them down again here to make it easier —

  1. We don't need to move page-specific components to the top-level components directory. I'm happy to rename the top level components directory to shared-components in case its confusing. Please undo these (e.g. InterlinksSection, ExportsAnalysis etc)
  2. A bunch of CSS has been deleted from client/components/Stat/Stat.scss — I'm not entirely sure why
  3. resolutions key from package.json has been deleted — this shouldn't be deleted
  4. pages/package/[...packageString]/index.page.js (and a few other page components) have been renamed to pages/package/[...packageString].page.js — this is un-necessary. By convention we nest all pages under a directory so we don't need to refactor them in case there are more components to be added in the same directory.

Also, to repeat — let's only do the minimum number of changes required for a smooth upgrade. It also helps us debug better in case something goes wrong after the update.

darasus commented 2 years ago

@pastelsky thanks for summarising you comments!

  1. This is not the case anymore, I've reverted these in the previous commits.
  2. Not sure what happened with Stat.scss, will revert it
  3. Reverted resolutions
  4. And reverted pages

How is it looking now?

pastelsky commented 2 years ago

Thanks for fixing all of the issues. This looks good to me.

As a next step, we can —

  1. Cleanup bundlephobia's babelrc so that Next JS can start using swc for compilation
  2. Think about using React server components to make bundlephobia even faster and fix SSR
pastelsky commented 2 years ago

Congratulations on your first successful contribution to bundlephobia.