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

WIP #38945

Draft
wants to merge 24 commits into
base: master
Choose a base branch
from
Draft

WIP #38945

wants to merge 24 commits into from

Conversation

JustFly1984
Copy link

@JustFly1984 JustFly1984 commented Apr 20, 2024

Refactoring Gatsby.

  • Updated Node to 20.12.2 LTS
  • pnpm instead of yarn.
  • typescript improvements
  • updating dependencies
  • adding missing dependencies.
  • updated lerna
  • had to delete deprecated packages and gatsby-plugin-flow package, maybe it is time to deprecate it. In npm there is no usage of last versions, so anyway I've refactored all flow files into typescript.
  • had to remove eslint-config-react-app, cos it is very rarely maintained and requires eslint@7
  • removed node-fetch and replaced it with globalThis.fetch
  • refactored gatsby-plugin-image to support cssnano@7

Fixing bugs:

  • CSP nonce suport for webpack generated files.
  • Monkeypatched console fix
  • fixed gatsby-cli cross linking

Still require to fix:

  • fix tests and run
  • Update types for redux and react-redux, maybe use redux-toolkit
  • Update xstate from 4 to 5
  • Update Eslint@9 and flat config. Also update gatsby-eslint-config to support eslint@9
  • get rid of lodash library
  • replace react-server-dom-webpack 0.0.1 with something supporting [email protected]
  • replace @graphiql/plugin-code-exporter 2.0.0. with something supporting [email protected]
  • replace @vercel/fetch-retry 5.1.3 with something not using node-fetch
  • replace @turist/fetch 7.2.0 with something not using node-fetch

running pnpm outdated -r
Screenshot 2024-04-25 at 21 29 02

peer dependencies issues:
Screenshot 2024-04-26 at 16 45 36

@JustFly1984 JustFly1984 requested a review from a team as a code owner April 20, 2024 13:49
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Apr 20, 2024
@danaiszuul
Copy link

@JustFly1984 We are reviewing this! Hope to have it merge very soon, our team is mostly in EU so it will be a few hours 🤞

@danaiszuul danaiszuul removed the request for review from jenae-janzen April 25, 2024 21:56
@JustFly1984
Copy link
Author

I'm still working on it, need to clean up some stuff, will have a commit later today.
Also I have not fixed tests yet and did not removed unused dependencies.

@pieh
Copy link
Contributor

pieh commented Apr 26, 2024

Hi @JustFly1984,

Thanks so much for all of the awesome work you've put into this Gatsby PR! We really appreciate your enthusiasm and all of improvements you've made.

This PR is pretty large so it’s going to be difficult for us to review it in its current format.

There are too many different features that you've worked on which is clouding our ability to understand all of the changes. We have to be mindful of regressions, additional bugs, and any security concerns so we want to ensure we can review this properly for you.

To make it easier for reviewers to dive in and give you the best feedback, could we consider splitting this up into smaller pull requests?

Here's what we are thinking:

  • Separate PRs for each specific change:
    • CSP nonce support for webpack generate files
    • Monkey-patched console fix
    • Gatsby CLI cross-linking
    • Flow to typescript refactor.
  • A clear description and explanation for each individual change would be fantastic!

For the other proposed changes:

  • Updating the minimum Node version is something we should avoid at the moment as the minimum Node version for the current version of Gatsby is 18. Let’s look at adding Node versions 20 and 22 to our matrix tests in a separate PR. It definitely seems sensible to include these changes but in a separate PR.
  • Changing the package manager is an interesting idea, but it would be helpful to understand the specific goals and potential trade-offs before diving in. Can you share the benefits you see from changing this and raise an issue for it for a community discussion? It’s a big change which affects any Gatsby contributors and it feels like it might be a lot of extra work so it would be good to understand why we might need this change.
  • Formatting changes can be tricky. While we can understand that some formatting styles are personal, changing our linting or formatting rules causes a change to most all the files in the repo and can introduce difficulties in following the changes in the git history. If you have some reasons why we should change the formatting then can you raise an issue so we can discuss it further?

By breaking things down a bit, we can get more focused reviews and make sure each change gets the attention it deserves.

It sounds like you've been thinking a lot about how to improve Gatsby! While these changes are great, is there anything else you've been considering that you think could make Gatsby even better? We'd love to hear your ideas! Feel free to open separate GitHub issues for any additional features or improvements you have in mind. That way, we can keep track of them and discuss them with the community. We really value your contributions and insights!

Thanks!

P.S. Once we have these new PRs we can review and get them merged with a smaller LGTM! ;)

@JustFly1984
Copy link
Author

Found one bug in packages/gatsby/src/schema/node-model.ts line 623, 624

Argument of type 'GatsbyIterable' is not assignable to parameter of type 'IGatsbyNode[]'.
Type 'GatsbyIterable' is missing the following properties from type 'IGatsbyNode[]': pop, push, join, reverse, and 28 more.

Screenshot 2024-04-26 at 22 13 43

@JustFly1984
Copy link
Author

JustFly1984 commented Apr 26, 2024

I've pushed one more commit with all @ts-ignore meaningfully commented. Added missing types.
Looks like it will be long weekend, will continue tomorrow.

@JustFly1984
Copy link
Author

I've updated cssnano and a lot of code in gatsby-plugin-image, pushed in last commit

@JustFly1984
Copy link
Author

Today refactored gatsby-plugin-image gatsby-plugin-manifest and gatsby-plugin-sharp.

@ascorbic
Copy link
Contributor

@JustFly1984 can you respond to @pieh's comment? If this is to be merged we'll need you to address those issues and break these into separate PRs rather than adding more to this one

@JustFly1984
Copy link
Author

@ascorbic @pieh I'm still working on it, typing redux.
The main difference issue is caused by adding semicolons, changing strings from backticks to double quotes, string length limit to 80.
I will revert it back and squash the PR, so there is only valid changes left.

I will revert node version to 18 also.

I can't really divide it into smaller PRs yet.
Today I started to bring some love to redux reducers and actions to improve types. Also refactoring jest tests to typescript.
Still work in progress.

@JustFly1984
Copy link
Author

JustFly1984 commented Apr 29, 2024

The only thing I don't want to revert is pnpm. It saves so much time on package crosslinking. I've even changed docs everywhere to use pnpm instead of yarn.

@ascorbic
Copy link
Contributor

Thanks. It's great that you're doing this, but I do think it will be easier for you if you split the parts earlier, rather than after implementing everything.

@JustFly1984
Copy link
Author

JustFly1984 commented Apr 29, 2024

I also want to rewrite some of downstream dependencies which uses node-fetch as peerDependencies.

I already got rid of node-fetch internally in gatsby repo.

Today I was auditing one of my gatsby projects, and there was 5 more security issues detected. I could resolve them successfully with yarn resolutions.

In general I feel it will be easier to release new major version than to divide the PR in small chunks. I've fixed a lot of stuff.
After I finish with typescript rewrite, it will be much more modern codebase to work with.

@JustFly1984
Copy link
Author

Btw, I will bring back deprecated packages too, to reduce the change scope.

@JustFly1984
Copy link
Author

Also I want to bring my friend to help me to rewrite xstate from 4 to 5 in 2 packages.

@JustFly1984
Copy link
Author

JustFly1984 commented Apr 29, 2024

@ascorbic @pieh The one more reason for me to do it - for years I'm tired of warnings and security issues in downstream packages while reinstalling or updating dependencies in my projects:

Screenshot 2024-04-29 at 20 32 47

@ascorbic
Copy link
Contributor

We're going to need at least one PR per change, so for example one PR for the CSP, one PR for the Flow to TypeScript changes, one for Redux types etc. Do not include any of the linting or package manager changes. We can start a discussion on the change to the package manager, but that is something that cannot be done before that. I'm a big fan of pnpm, but we have a lot of tooling with yarn and lerna, so it's not something to change lightly. We're not going to change the linting or formatting rules because that means lots of changes with no real benefit.

I strongly recommend you don't try to work on any other parts until we've been able to review the first PRs, or you'll need to deal with merge issues etc, as we won't be able to review each of them until the others have been merged.
I understand the desire to get down to work on it, but it's a lot of work to review PRs of this size, and it's not feasible unless they are provided as one PR per change. This is a rule that every project will follow, and is particularly important with large open source projects like Gatsby.

@ascorbic ascorbic marked this pull request as draft April 29, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants