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

Move package to ESM + ESLint 9 #435

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

ahkhanjani
Copy link
Contributor

In this PR I have:

  • Made the npm package to use ESM internally (to support ESLint 9).
  • Updated ESLint dependencies to latest and added some rules to be more strict. Removing the outdated no-restricted-imports plugin caught a problem on its own.
  • Fixed the lint errors.

Copy link

vercel bot commented Nov 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-resizable-panels ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 18, 2024 10:51pm

@ahkhanjani
Copy link
Contributor Author

@bvaughn Is this PR ok with you?

E2E tests are failing because of commonjs tests. So how about we drop cjs entirely and release a v3? I can do it if it's ok with you

@ahkhanjani ahkhanjani marked this pull request as ready for review November 17, 2024 12:07
Copy link
Owner

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

I appreciate the effort spent on this PR. It's kind of a noisy change though, and (as it is) I think it could cause a couple of small regressions (commented). I'm not sure if I want to move forward with it.

@ahkhanjani
Copy link
Contributor Author

@bvaughn What should we do about the cjs issue? Should we drop cjs support and go for a v3?

@bvaughn
Copy link
Owner

bvaughn commented Nov 17, 2024

Hmm. I don't think there's a strong reason to drop CJS support, unless it would in some way dramatically improve the source code/maintainence?

@ahkhanjani
Copy link
Contributor Author

Hmm. I don't think there's a strong reason to drop CJS support, unless it would in some way dramatically improve the source code/maintainence?

My knowledge on the topic of CJS/ESM is limited but since the ecosystem has been moving on from CJS and been dropping support since Node 16 got deprecated (I'm not sure why but that's what I've witnessed), the project itself has to use ESM ("type": "module" in package.json) to support the latest releases and be up-to-date. So as you can see because I've switched to ESM internally to support ESLint 9, the E2E tests related to CJS are failing. Maybe we can make them work by adding more complexity but that would make the project harder to maintain. So my answer is yes, it will improve the maintenance by a lot.

Also I think people who use React mostly use ESM and the ones who aren't up to date will be ok with v2.

@ahkhanjani
Copy link
Contributor Author

Hey @bvaughn. I fixed an import issue I had made and now one test is strangely failing:

PanelGroup › invariants › should throw if default size is less than 0 or greater than 100

Expected substring: "Invalid layout"                                                                                                               
Received message:   ""

The test itself makes sense but I couldn't find anywhere in the code that throws this exception. Why wasn't the error failing before?

@ahkhanjani
Copy link
Contributor Author

I'm also getting warnings saying import { act } from "react-dom/test-utils" is deprecated in favor of import { act } from "react". Is it ok if I replaced those? No issues were caused when I tested it.

One other thing I'm seeing is the exported createRef from vendor/react.ts is only being used in tests and not in the code so it appears in the bundle without being used. Should I change the imports in tests to react directly and remove the export from vendor?

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.

2 participants