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

chore(deps): updated practically all dependencies #198

Merged
merged 20 commits into from
Sep 5, 2022

Conversation

qeleb
Copy link
Contributor

@qeleb qeleb commented Aug 18, 2022

Modernizing

I started out using this boilerplate over two months ago. As I gradually improved my own project, I decided it would be a good idea to send some of those changes back. So, after days of testing... I have made a handful of improvements with the aim to modernize this boilerplate.

Improvements

  1. Got rid of warnings during yarn install by being explicit about dependencies Eslint & Jest

    Edit: this will come in another pull request along with an upgrade to yarn v3.

  2. Updated 46 dependencies in package.json & template.json

    I did not simply update to the newest version, but the version which best fits with the other dependencies. This results in a much more optimized dependency graph. This is why the yarn.lock file is 119 lines shorter than before, despite loads of new dependencies. It will get even smaller when the boilerplate is upgraded from yarn v1.

  3. Updated react-router from v5 to v6, bringing its new syntax. This change required updating the snapshot for Jest & updating the documentation.

  4. Added Windows support to internal scripts by opting for platform independent syntax.

  5. Removed the use of Redux-Toolkit's deprecated getDefaultMiddleware(), which has been replaced by the modern syntax, introduced in v1.4.0 (June 2020)

  6. Removed vs code workspace recommendations for deprecated chrome debugger & deprecated npm

    This functionality has been built-in since VS Code v1.46 (May 2020)

  7. Added a workspace recommendation for Jest & styled-components to replace the deprecated (now removed) extension.

Dependencies

  1. I tested a huge amount of versions for every dependency before settling on these. I think these are the best for now, although many of these dependencies will work just fine on newer versions. I think newer is not necessarily better.

  2. An upgrade to yarn v3 will be required to avoid some of the warnings that users experience on each yarn install. This is probably necessary and should come soon.

Testing

Please take a look at src\app\pages\HomePage\Features\GithubRepoForm\__tests__\index.test.tsx. This test gave me some trouble, but it appeared to be broken already before I made any changes.

@coveralls
Copy link

coveralls commented Aug 18, 2022

Pull Request Test Coverage Report for Build 2989475613

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 99.679%

Totals Coverage Status
Change from base Build 2529374304: 0.001%
Covered Lines: 250
Relevant Lines: 250

💛 - Coveralls

@Can-Sahin
Copy link
Member

@Markkos89
Copy link

hey @CrunchyCat great work here! thanks for sharing.
I would only kindly suggest to update the docs/building-blocks/routing.md file, accordingly to the new version of react-router-dom

image

The Usage part would be something like:

To add a new route, simply import the Route component and use it standalone or inside the Switch component (all part of
RR5 API):

      <Route path="/" element={<HomePage />} />

Top level routes are located in src/app/index.tsx.

And the Child Routes part would be like:

AboutPage/index.tsx

import { Switch, Route } from 'react-router-dom';

export function AboutPage() {
  return (
    <Routes>
      <Route path="/about/our-team" />
    </Routes>
  );
}

Thanks again for the amazing work and best regards!

@qeleb
Copy link
Contributor Author

qeleb commented Aug 21, 2022

https://github.com/react-boilerplate/react-boilerplate-cra-template/runs/7897720487?check_suite_focus=true

eslint fails ?

Okay, I believe I've fixed it. The problem is with the way that yarn v1 resolves eslint as a direct dependency & a dependency of react-scripts. I strongly recommend that we switch the project to yarn v3 for a host of benefits, including having fewer dependencies, more readable & verbose output, and having fewer warnings caused by the somewhat janky way we are avoiding using yarn v3.

I expect that it would probably fail the build on Jest as well, so I removed it as a direct dependency for now so that it will pass the tests. This will result in some annoying warnings during yarn install.

I will create another PR soon, adding back Eslint & Jest as direct dependencies along with yarn v3.

@qeleb
Copy link
Contributor Author

qeleb commented Aug 22, 2022

@Can-Sahin @Markkos89 Would you two take a look at this PR? I'm all done for now and excited to see it merged.

@Can-Sahin
Copy link
Member

Can-Sahin commented Aug 22, 2022

'yarn test' fails with weird errors and it logs so much to the console.

I googled a bit and literally, nothing comes up. It scares me :)

I also copy pasted the older versions of packages and still same error weird enough.

@qeleb
Copy link
Contributor Author

qeleb commented Aug 23, 2022

'yarn test' fails with weird errors and it logs so much to the console.

I googled a bit and literally, nothing comes up. It scares me :)

I also copy pasted the older versions of packages and still same error weird enough.

that’s so strange ): I have no idea why that would be happening. as before, the build works correctly for me. 😔

unless you are talking about the yarn test that used to fail because the test file is missing act()

@Can-Sahin
Copy link
Member

Can-Sahin commented Aug 23, 2022

This cookie error happens in my local computer too. Are you testing after creating a CRA app from the template?

yarn create:cra-app creates the app and you should test those actually. Are you doing the same?

like: yarn create:cra-app -> mv generated-cra-app ~/somewhere outside the repo -> cd generated-cra-app -> yarn test -> all tests fail

@qeleb
Copy link
Contributor Author

qeleb commented Aug 29, 2022

This cookie error happens in my local computer too. Are you testing after creating a CRA app from the template?

yarn create:cra-app creates the app and you should test those actually. Are you doing the same?

like: yarn create:cra-app -> mv generated-cra-app ~/somewhere outside the repo -> cd generated-cra-app -> yarn test -> all tests fail

Yes, I am doing this also. As before, the exact same tests which used to fail before any changes, are the only ones that fail now. Everything works perfectly. What version of Node are you using to test? Maybe there is some kind of strange difference for an older version of Nodejs?

@Can-Sahin
Copy link
Member

My node is 16. Also CI tests run on various node versions.

Non of the tests were giving this error before. Literally all of them fail now and I cannot find anything online. There must be something super small and weird. I ll try to apply each update one by one and run tests in between to see which one causes this problem

@qeleb
Copy link
Contributor Author

qeleb commented Aug 30, 2022

My node is 16. Also CI tests run on various node versions.

Non of the tests were giving this error before. Literally all of them fail now and I cannot find anything online. There must be something super small and weird. I ll try to apply each update one by one and run tests in between to see which one causes this problem

Okay. thank you. When I get a chance, I will test on Nodejs 14. I still have no idea what the problem could be.

At the very least... the internal script upgrades, vs code workspace recommendations, dependency minor updates, and removal of deprecated syntax are safe changes that I could make a separate PR for. Although, I'm really hoping this can be sorted out soon and merged as a whole.

@Can-Sahin
Copy link
Member

Thanks for the effort 👍 I want to merge it as well, but I need to figure out what's wrong. It's weird. Because as soon as I merge I will publish a new version and I don't want to deal with 10s of people coming back saying it doesn't work, because who knows what environment settings they have :)

@Can-Sahin
Copy link
Member

I am merging this. Are you finished? I see some commits recently.

@qeleb
Copy link
Contributor Author

qeleb commented Sep 5, 2022

I am merging this. Are you finished? I see some commits recently.

I would love if you do! Then I can shift my focus to fixing some of the minification issues, switching to yarn 3, and adding the yarn typescript plugin. Although, didn’t you say it’s causing you errors? Did we ever figure out why?

I was just trying to figure out what might be causing the errors you were seeing but i’m still not able to see them.

@Can-Sahin
Copy link
Member

Can-Sahin commented Sep 5, 2022

They are gone. It fails in node 17 in CI/CD but I cannot figure out why. Most of them disappeared and I assume it was a bug in some packages used. Its the biggest problem with CRA. It fails randomly because the versions are not fixed. If some super deep packages publishes a faulty release everything is failing.

Thanks a lot. Glad to have these

@Can-Sahin Can-Sahin merged commit 8c2d9d1 into react-boilerplate:master Sep 5, 2022
@Can-Sahin
Copy link
Member

All green now. You also could add yourself to the contributors list ;)

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.

None yet

4 participants