Closed JustFly1984 closed 3 months ago
@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 🤞
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.
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:
For the other proposed changes:
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! ;)
Found one bug in packages/gatsby/src/schema/node-model.ts
line 623, 624
Argument of type 'GatsbyIterable
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.
I've updated cssnano and a lot of code in gatsby-plugin-image, pushed in last commit
Today refactored gatsby-plugin-image gatsby-plugin-manifest and gatsby-plugin-sharp.
@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
@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.
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.
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.
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.
Btw, I will bring back deprecated packages too, to reduce the change scope.
Also I want to bring my friend to help me to rewrite xstate from 4 to 5 in 2 packages.
@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:
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.
As discussed above, please feel free to open smaller, more easily reviewable PRs and we'll be happy to take a look. Thanks!
Refactoring Gatsby.
Fixing bugs:
Still require to fix:
running
pnpm outdated -r
peer dependencies issues: