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

Fix module resolution issues in dual CJS/ESM setup #264

Merged
merged 9 commits into from
Apr 16, 2024
Merged

Fix module resolution issues in dual CJS/ESM setup #264

merged 9 commits into from
Apr 16, 2024

Conversation

wojtekmaj
Copy link
Contributor

@wojtekmaj wojtekmaj commented Oct 12, 2023

This PR:

  • Marks the package as ESM-first
  • Ensures backwards compatibility by keeping all CJS files, keeping UI and sidecar pseudo-packages for Node.js 10 compatibility, and adding package.json in dist/cjs to override the above change
  • Uses package.json's exports to ensure all entry points are still valid - in all modes
  • Moves all types to /src, so that they sit alongside the modules they belong to, and so that they are copied separately to /dist/cjs and /dist/es2015 directories.

arethetypeswrong CLI shows a clear improvement:

Before:

react-focus-lock % attw react-focus-lock-v2.11.3.tgz

react-focus-lock v2.11.3

🤨 CommonJS module simulates a default export with exports.default and exports.__esModule, but does not also set module.exports for compatibility with Node. Node, and some bundlers under certain conditions, do not respect the __esModule marker, so accessing the intended default export will require a .default property access on the default import. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/CJSOnlyExportsDefault.md


"react-focus-lock"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🤨 CJS default export
bundler: 🤨 CJS default export

***********************************

"react-focus-lock/UI"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🤨 CJS default export
bundler: 🤨 CJS default export

***********************************

"react-focus-lock/sidecar"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🤨 CJS default export
bundler: 🤨 CJS default export

***********************************

After:

react-focus-lock % attw react-focus-lock-v2.11.3.tgz

react-focus-lock v2.11.3

 No problems found 🌟


"react-focus-lock"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🟢 (ESM)
bundler: 🟢 

***********************************

"react-focus-lock/sidecar"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🟢 (ESM)
bundler: 🟢 

***********************************

"react-focus-lock/UI"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🟢 (ESM)
bundler: 🟢 

***********************************

Fixes #257

UI/UI.d.ts Outdated Show resolved Hide resolved
UI/package.json Outdated Show resolved Hide resolved
sidecar/package.json Outdated Show resolved Hide resolved
src/UI.d.ts Outdated Show resolved Hide resolved
src/index.d.ts Outdated Show resolved Hide resolved
src/interfaces.d.ts Show resolved Hide resolved
src/interfaces.d.ts Outdated Show resolved Hide resolved
src/sidecar.d.ts Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is an exact copy of preexisting sidecar.d.ts.

@theKashey theKashey self-requested a review October 13, 2023 05:37
@theKashey
Copy link
Owner

Give me some time to test integratons.

@wojtekmaj
Copy link
Contributor Author

One improvement that comes to my mind is that .d.ts files sitting in pseudorepo /sidecar and /UI directories could reexport type definitions from /dist/cjs instead.

@theKashey
Copy link
Owner

I would really like to rewrite focus-lock in TS and don't worry about manual type declarations. So, don't worry about them 😉

@theKashey
Copy link
Owner

Oh, time flies and I am still buried in work. Sorry, should become more free closer to the new year.

@wojtekmaj
Copy link
Contributor Author

Don't worry mate. No one has the right to expect anything from you for $0.

Copy link

stale bot commented Feb 13, 2024

This issue has been marked as "stale" because there has been no activity for 2 months. If you have any new information or would like to continue the discussion, please feel free to do so. If this issue got buried among other tasks, maybe this message will reignite the conversation. Otherwise, this issue will be closed in 7 days. Thank you for your contributions so far.

Copy link

stale bot commented Apr 14, 2024

This issue has been marked as "stale" because there has been no activity for 2 months. If you have any new information or would like to continue the discussion, please feel free to do so. If this issue got buried among other tasks, maybe this message will reignite the conversation. Otherwise, this issue will be closed in 7 days. Thank you for your contributions so far.

@stale stale bot added the state label Apr 14, 2024
@wojtekmaj
Copy link
Contributor Author

Bad bot!

@stale stale bot removed the state label Apr 14, 2024
@theKashey
Copy link
Owner

I am a little hesitant to move forward due to the problem highlighted at gregberge/loadable-components#1003

  • single package expected to be a "singleton"
  • being imported from ESM and CJS
  • ending as two different packages imported and not knowing about each other

This moves change from "just here" to "all consumers".

The "safe" way forward lies in refactoring of react-clientside-effect in order to make it "duplication aware", for example via tracking singletons via stable(string) keys.


The question I dont have an answer for - what is required from downstream packages in order to make this one ESM-compatible.
Ie, do I need to esm react-clientside-effect, potentially causing the same "duplication" to it, or can keep it as is, making it easier to track references.

@wojtekmaj - I really hope you have some answers for the questions above, well the one question - managing "singleton" in ESM packages.

@wojtekmaj
Copy link
Contributor Author

@theKashey - The approach I took with dual ESM/CJS config presented here was battle tested in React-PDF, React-Calendar, React-Date-Picker and many other packages I maintain.

I do understand your concerns though.

react-clientside-effect looks like a fairly simple module ESM should have no issues with. It might be converted to dual CJS/ESM, but doesn't need to be. I confirmed this by running an ESM version of react-focus-lock from this PR in pure Node.js, and checked that withSideEffect is still a function and not e.g. { default: [Function: withSideEffect] }.

@wojtekmaj
Copy link
Contributor Author

I rebased the PR off latest master, and edited the first comment, since now the PR is making everything green, not just ESM module resolution!

@theKashey
Copy link
Owner

and checked that withSideEffect is still a function

More about there is a possibility to reach withSideEffect via require path and then via import path, resulting in two different worlds, not one singlenton.

Could affect only cases with multiple modals being open using different ways to open them. Not sure how "real" this case is, but it's the only moment that separates this package from others.

@wojtekmaj
Copy link
Contributor Author

Not sure how "real" this case is

Most bundlers will probably use ESM, because they are capable of resolving ESM. Even if the modules are CJS, they are "wrapped" so that they could all talk to each other seamlessly.
If you don't use any bundler at all, you're forced to use ESM, because CJS doesn't work outside of Node.
I may be missing something here, but I don't see how you'd end up with two versions of react-focus-lock. Perhaps in a Node.js-only application, where you could actually use both import in ESM and require in CJS, but that's not a realistic scenario for a front-end package, and also, if you write both ESM and CJS in one Node.js project, you really should reconsider your life choices.

@theKashey
Copy link
Owner

I tend to agree - the problem with loadable-components happened at the server side and FocusLock is a purely front-end component, including the "dangerous part" being named as react-CLIENTSIDE-effect

@theKashey
Copy link
Owner

It will take a few days to test it as deeply as I possibly could...

@wojtekmaj+++ and 🙇 for all your effort and impact.

@theKashey theKashey merged commit 871e8f3 into theKashey:master Apr 16, 2024
1 check failed
@wojtekmaj wojtekmaj deleted the esm branch April 16, 2024 04:40
nickspaargaren pushed a commit to nickspaargaren/react-focus-lock that referenced this pull request Dec 11, 2024
Fix module resolution issues in dual CJS/ESM setup
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.

Issue with importing FocusLock component
2 participants