Skip to content

chore: remove react-scripts and jest #25383

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

Merged
merged 5 commits into from
Jan 19, 2023
Merged

Conversation

ZachJW34
Copy link
Contributor

@ZachJW34 ZachJW34 commented Jan 6, 2023

User facing changelog

na

Additional details

Remove jest from the monorepo and redundant webpack-preprocessor tests.

We already have system-tests for webpack-preprocessor that extend webpack. We should avoid having packages that install test dependencies (like react-scripts which was including jest) and utilize system-tests.

Size of node_modules before/after change:

Before: 2610 MB
After: 2446 MB

Steps to test

na

How has the user experience changed?

na

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?

Sorry, something went wrong.

@cypress
Copy link

cypress bot commented Jan 6, 2023



Test summary

458 0 1 0Flakiness 0


Run details

Project cypress
Status Passed
Commit f423588
Started Jan 19, 2023 9:57 PM
Ended Jan 19, 2023 10:04 PM
Duration 06:52 💡
OS Linux Debian -
Browser Electron 106

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@ZachJW34 ZachJW34 marked this pull request as ready for review January 17, 2023 18:19
command: yarn test
working_directory: npm/webpack-preprocessor/examples/use-ts-loader
- run:
name: Start React app
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, I looked at system tests, I see

image

We've also got a few system-tests using .babelrc so I don't think we are losing any coverage.

@@ -7,7 +7,7 @@
"lint": "eslint --ext .js,json,.eslintrc .",
"lint-changed": "node ./lib/scripts/lint-changed",
"lint-fix": "npm run lint -- --fix",
"test": "jest"
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@@ -4,5 +4,4 @@ node_modules/
_test-output
**/cypress/videos
cypress/screenshots
examples/react-app/build/
Copy link
Member

Choose a reason for hiding this comment

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

We might want to keep these .gitingnore paths around? Stale branches pulling in this change will likely might have these git ignore files laying around and git will suggest we check them in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll revert! Not harming anyone and less confusion for devs.

@@ -42,12 +42,6 @@ module.exports = (on) => {
}
```

## Examples
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth listing the system-test examples we have in place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a few examples back.

@@ -59,9 +59,6 @@
"mocha": "^7.1.0",
"mockery": "2.1.0",
"proxyquire": "2.1.3",
Copy link
Member

Choose a reason for hiding this comment

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

Can we also drop @babel/plugin-proposal-nullish-coalescing-operator? It was only used in use-babel example AFAIK

Just looking through, maybe out of scope, but looks like these might not be used anymore in this module:

  • @fellow/eslint-plugin-coffee
  • @typescript-eslint/eslint-plugin
  • @typescript-eslint/parse

Might be a few others as well that can now rely on the root level dev deps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll take a look later today, I love removing unused deps!

Copy link
Member

Choose a reason for hiding this comment

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

me too 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned up a few more! Could spend a week+ cleaning up all of the unused deps but I just focused on webpack-preprocessor (and batteries included).

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a bunch of packages to automate removed old deps eg https://www.npmjs.com/package/depcheck. Probably worth doing at some point, should be easy enough to run a few and see what it recommends removing.

Copy link
Member

@emilyrohrbough emilyrohrbough left a comment

Choose a reason for hiding this comment

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

Vite-dev-server's .estlintrc file includes

"globals": {
    "jest": "readonly"
  },

this might also be stale given there are no jest tests in vite-dev-server.

emilyrohrbough and others added 4 commits January 19, 2023 09:23

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@ZachJW34 ZachJW34 changed the title chore: remove react-scripts and jest [run ci] chore: remove react-scripts and jest Jan 19, 2023
@ZachJW34 ZachJW34 merged commit 8b1d74f into develop Jan 19, 2023
@ZachJW34 ZachJW34 deleted the zachw/remove-jest-react-scripts branch January 19, 2023 22:07
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

3 participants