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

Issue with importing FocusLock component #257

Closed
barroit opened this issue Aug 22, 2023 · 18 comments · Fixed by #264
Closed

Issue with importing FocusLock component #257

barroit opened this issue Aug 22, 2023 · 18 comments · Fixed by #264
Assignees

Comments

@barroit
Copy link

barroit commented Aug 22, 2023

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:

  1. Create a new Remix project by using npx create-remix@latest, with default name, and choose "Just the basics", then the "Express server" deploy target, use TypeScript, run npm install.
  2. Install react-focus-lock via npm i react-focus-lock.
  3. Import the FocusLock component in a React component file with import FocusLock from "react-focus-lock";
  4. Try to use the FocusLock component in the JSX code as .
  5. Observe that the component does not work as expected and requires usage as <FocusLock.default>.

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:

import React from "react";
import FocusLock from "react-focus-lock";

function MyComponent() {
  return (
    <FocusLock.default>
      <input type="text" />
      <button>Click me</button>
    </FocusLock.default>
  );
}

export default MyComponent;

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:

import("react-focus-lock").then(pkg => {
  console.log(pkg);
});

this will output:

[Module: null prototype] {
  AutoFocusInside: [Function: AutoFocusInside] {
    propTypes: {
      children: [Function: bound checkType],
      disabled: [Function],
      className: [Function]
    }
  },
  FocusLockUI: {
    '$$typeof': Symbol(react.forward_ref),
    render: [Function: FocusLockUI],
    propTypes: {
      children: [Function],
      disabled: [Function],
      returnFocus: [Function],
      focusOptions: [Function],
      noFocusGuards: [Function],
      hasPositiveIndices: [Function],
      allowTextSelection: [Function],
      autoFocus: [Function],
      persistentFocus: [Function],
      crossFrame: [Function],
      group: [Function],
      className: [Function],
      whiteList: [Function],
      shards: [Function],
      as: [Function],
      lockProps: [Function],
      onActivation: [Function],
      onDeactivation: [Function],
      sideCar: [Function: bound checkType]
    },
    defaultProps: {
      children: undefined,
      disabled: false,
      returnFocus: false,
      focusOptions: undefined,
      noFocusGuards: false,
      autoFocus: true,
      persistentFocus: false,
      crossFrame: true,
      hasPositiveIndices: undefined,
      allowTextSelection: undefined,
      group: undefined,
      className: undefined,
      whiteList: undefined,
      shards: undefined,
      as: 'div',
      lockProps: {},
      onActivation: undefined,
      onDeactivation: undefined
    }
  },
  FreeFocusInside: [Function: FreeFocusInside] {
    propTypes: { children: [Function: bound checkType], className: [Function] },
    defaultProps: { className: undefined }
  },
  InFocusGuard: [Function: InFocusGuard] {
    propTypes: { children: [Function] },
    defaultProps: { children: null }
  },
  MoveFocusInside: [Function: MoveFocusInside] {
    propTypes: {
      children: [Function: bound checkType],
      disabled: [Function],
      className: [Function]
    },
    defaultProps: { disabled: false, className: undefined }
  },
  __esModule: true,
  default: {
    default: {
      '$$typeof': Symbol(react.forward_ref),
      render: [Function: FocusLockUICombination],
      propTypes: [Object]
    },
    FocusLockUI: [Getter],
    AutoFocusInside: [Getter],
    MoveFocusInside: [Getter],
    useFocusInside: [Getter],
    FreeFocusInside: [Getter],
    InFocusGuard: [Getter]
  },
  useFocusInside: [Function: useFocusInside]
}
@theKashey
Copy link
Owner

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

@wojtekmaj
Copy link
Contributor

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

@theKashey
Copy link
Owner

Reminds me alexreardon/memoize-one#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 alexreardon/memoize-one#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

@wojtekmaj
Copy link
Contributor

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.

@theKashey
Copy link
Owner

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

While 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.

@theKashey theKashey self-assigned this Aug 29, 2023
@barroit
Copy link
Author

barroit commented Sep 2, 2023

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

@barroit barroit closed this as not planned Won't fix, can't repro, duplicate, stale Sep 2, 2023
@theKashey
Copy link
Owner

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"));

@artursvonda
Copy link

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.

@theKashey
Copy link
Owner

Probably the only feasible solution is to deprecate default export and expose named one.

@theKashey theKashey reopened this Oct 12, 2023
@wojtekmaj
Copy link
Contributor

wojtekmaj commented Oct 12, 2023

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:

image

Vite happy:

image

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).

@theKashey
Copy link
Owner

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

Copy link

stale bot commented Dec 12, 2023

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 Dec 12, 2023
@wojtekmaj
Copy link
Contributor

Definitely a bump, especially since #264 is up and TS rewrite may be coming.

@stale stale bot removed the state label Dec 12, 2023
Copy link

stale bot commented Feb 10, 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 Feb 10, 2024
@wojtekmaj
Copy link
Contributor

Bad bot!

@stale stale bot removed the state label Feb 10, 2024
@theKashey
Copy link
Owner

I hope this issue will be resolved in a matter of days

Copy link

stale bot commented Apr 11, 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 11, 2024
@wojtekmaj
Copy link
Contributor

Bump

@stale stale bot removed the state label Apr 11, 2024
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 a pull request may close this issue.

4 participants