Closed barroit closed 4 months ago
Wondering what Remix is doing with default exports, if that is Remix's doing.
default
looks like how module should look like -FocusLockUI: [Getter]
, where Getter
is important.
The stuff outside default
is a bit strange. It should not be there.
There is nothing I can "fix" from focus-lock side and there is no other way I can export default export except exporting it as the default export - https://unpkg.com/browse/react-focus-lock@2.9.5/dist/es2015/index.js
It's nothing Remix specific, it's actually wrong. Here's another tool confirming it:
https://arethetypeswrong.github.io/?p=react-focus-lock%402.9.5
Reminds me https://github.com/alexreardon/memoize-one/issues/116
TypeScript can understand what to do with default export, but naked node not always can, and fixing it will break typescript based projects (see https://github.com/alexreardon/memoize-one/issues/37)
The problem is that this case is only applicable to require
, not to import
. So something is wrong with configuration / babel / ts / you name it.
The only possible fix is to expose named export in addition to the default one in order not to introduce a breaking change
I don't think that's true. Package could be made ESM compatible without breaking changes.
The solution would be to ship separate types for ESM and CJS builds. The issue we're having here is mostly coming from types shared between these builds.
Ok, so let's step aside from handwritten types and imagine this library is written in TypeScript and TS manages both output of code and types
.default
export for cjs - https://unpkg.com/browse/focus-lock@0.11.6/dist/es5/index.jsWhile solution you talking about exists - it is a bad solution creation a lot of ambiguity and maintenance toll. Named import - it just works because it's not a part of a problem by design and one don't have to do anything.
In terms of Satisficing - you don't need to look for complex solution if you have a better solution.
I think the problem may be from remix. @uiw/React-codemirror has the same issue, one temporary solution is to import the library like:
import ReactFocusLock from "react-focus-lock";
const FocusLock = (ReactFocusLock as any).default as typeof ReactFocusLock;
and use in jsx like:
{ FocusLock ? (
<FocusLock>
{ children }
</FocusLock>
) : (
<ReactFocusLock>
{ children }
</ReactFocusLock>
) }
It's really weird
Too complicated:
import ProbablyReactFocusLock from "react-focus-lock";
const ReactFocusLock = ((ProbablyReactFocusLock as any).default ?? ProbablyReactFocusLock) as typeof ProbablyReactFocusLock;
But the problem is with the transpiler. This is the code it should generate for import
import Trap from './Trap';
// ⬇️
var _interopRequireDefault = require("@babel/runtime/helpers/interopRequireDefault");
var _Trap = _interopRequireDefault(require("./Trap"));
See the code - https://github.com/babel/babel/blob/b240d3f5b78b3dcda87311e3e0f9491fb570963e/packages/babel-helpers/src/helpers.ts#L370
TypeScript will do the same
import tabHook from './tabHook';
// ⬇️
var tabHook_1 = (0, tslib_1.__importDefault)(require("./tabHook"));
Came across same issue in my project (not using Remix, custom but fairly standard Webpack build) and I do think there's one issue we can address. Right now CJS build is pretending to be ESM package (adding __esModule: true
to exports). And the build system for some reason is not picking this up but instead is forcing CJS
module which causes the .default
issue. Since we do have separate ESM build, I think it makes sense to configure build so that CJS builds are pure CJS modules without pretending to be ESM. That might be breaking change but it actually would be the "right" solution. We could additionally add exports
key to package.json
to guide tools further into selecting the right file.
Probably the only feasible solution is to deprecate default export and expose named one.
Looking at the package, it seems like:
/dist/cjs
contains files compiled to CommonJS, with additional helpers (__esModule: true) to make ES module interop easier for other tools.
/dist/es2015
contains files that are pure ES modules.
However, the latter are invalid, because neither the package, nor the /dist/es2015
directory is marked as ES module. This confuses the tools.
It looks like this solves the issue for me:
package.json
:
"jsnext:main": "dist/es2015/index.js",
"module": "dist/es2015/index.js",
"types": "react-focus-lock.d.ts",
+ "type": "module",
+ "exports": {
+ ".": {
+ "types": "./react-focus-lock.d.ts",
+ "import": "./dist/es2015/index.js",
+ "require": "./dist/cjs/index.js",
+ "default": "./dist/es2015/index.js"
+ }
+ },
"sideEffects": [
"**/sidecar.js"
],
AND, because "type": "module"
is now set, we need an override not to break stuff for these 3 dinosaurs building front-end with require():
dist/cjs/package.json
:
{
"type": "commonjs"
}
The result?
TypeScript happy:
Vite happy:
Next.js happy too (screenshot would be looking the same so I don't bother uploading).
And no breaking changes that would have been caused by removal of default export!
That's the pattern I use with react-pdf
, react-calendar
, react-date-picker
and many other packages I maintain, and received zero complains so far :) I mean, regarding module resolution, of course. :D
One thing that I would not be happy about in this setup is how the types are generated (or rather, that they are not), so this still causes some tools to complain (like arethetypeswrong).
And no breaking changes that would have been caused by removal of default export!
Deprecation. I am not huge fan of default exports, so exposing a naming one along with preserved default one is a good way forward.
The result?
👍 look like we can kill two birds with one stone
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.
Definitely a bump, especially since #264 is up and TS rewrite may be coming.
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.
Bad bot!
I hope this issue will be resolved in a matter of days
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.
Bump
Description:
I'm experiencing an issue when importing the FocusLock component from the react-focus-lock library. Instead of being able to use the component as
<FocusLock>
, I have to use it as<FocusLock.default>
.Steps to reproduce:
npx create-remix@latest
, with default name, and choose "Just the basics", then the "Express server" deploy target, use TypeScript, run npm install.npm i react-focus-lock
.Expected behavior:
I expect to be able to use the FocusLock component as
<FocusLock>
without needing to specify the.default
property.Actual behavior:
The FocusLock component does not work as expected and requires usage as
<FocusLock.default>
.Code sample:
Environment:
Remix version: 1.19.3 react-focus-lock version: 2.9.5 Browser: Chrome 116.0.5845.96 Operating System: Ubuntu 22.04.3 LTS
Additional information:
this will output: