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

instantsearch.js: Add 'exports' field to package.json for proper module resolution in SSR, Jest, and other environments #6513

Open
1 task done
kevin-riveros opened this issue Jan 6, 2025 · 3 comments
Labels
triage Issues to be categorized by the team

Comments

@kevin-riveros
Copy link

🐛 Current behavior

Currently, when using history router in SSR with InstantSearch.js or testing with Jest, I need to manually patch the package.json to add the "exports" field. The current implementation requires me to modify the package.json to include the necessary paths for require and import. Without this patch, when importing like so:

import { history } from 'instantsearch.js/es/lib/routers';
  • In SSR, it doesn’t work because it’s trying to use the ES module (es/) path instead of the CommonJS (cjs/) path. This results in a failure to load the module.
  • In Jest, since Jest uses CommonJS by default, it fails to resolve the ES module (es/) path and cannot find the module, leading to test failures.

This issue violates current standards for Node.js and package management. Without the proper "exports" field, the package can fail in other scenarios, not just SSR or Jest. It may not work in certain environments or bundlers that expect explicit CommonJS and ES module paths in the exports field, leading to potential issues for other developers or integrations that rely on standard package configurations.

🔍 Steps to reproduce

  1. Install instantsearch.js in a React SSR project.

  2. Import the history router like so:

import { history } from 'instantsearch.js/es/lib/routers';
  
<InstantSearch
      // ...
     routing={{
           router: history({
               windowTitle: () => {},
	       push: () => {}
           })
     }
 >
  // Content
</InstantSearch>
  1. Attempt to render the SSR page.

  2. The page will crash because it is trying to load the module from the ES path instead of the CommonJS path.

  3. Run Jest tests in the project.

  4. Jest will fail because it attempts to resolve the ES module path instead of the CommonJS path.

  5. Manually patch the package.json to add the "exports" field as follows:

"exports": {
    ".": {
      "types": "./es/index.d.ts",
      "require": "./cjs/index.js",
      "import": "./es/index.js"
    },
    "./routers": {
      "types": "./es/lib/routers/index.d.ts",
      "require": "./cjs/lib/routers/index.js",
      "import": "./es/lib/routers/index.js"
    },
   // ...
  }
  1. After adding this patch, SSR and Jest tests work as expected.

Live reproduction

.

💭 Expected behavior

The package should define the "exports" field in its package.json to specify both require and import paths for CommonJS and ES module resolution. This would ensure that the module works in all environments, including SSR, Jest, and other bundlers, without needing to manually patch the package.json.

The "exports" field should look something like this:

"exports": {
    ".": {
      "types": "./es/index.d.ts",
      "require": "./cjs/index.js",
      "import": "./es/index.js"
    },
    "./routers": {
      "types": "./es/lib/routers/index.d.ts",
      "require": "./cjs/lib/routers/index.js",
      "import": "./es/lib/routers/index.js"
    },
    "./helpers": {
      "types": "./es/lib/helpers/index.d.ts",
      "require": "./cjs/lib/helpers/index.js",
      "import": "./es/lib/helpers/index.js"
    },
    "./widgets": {
      "types": "./es/lib/widgets/index.d.ts",
      "require": "./cjs/lib/widgets/index.js",
      "import": "./es/lib/widgets/index.js"
    },
    "./connectors": {
      "types": "./es/lib/connectors/index.d.ts",
      "require": "./cjs/lib/connectors/index.js",
      "import": "./es/lib/connectors/index.js"
    },
    "./types": {
      "types": "./es/types/index.d.ts",
      "require": "./cjs/types/index.js",
      "import": "./es/types/index.js"
    }
  }

This would resolve the issue in all environments and conform to current package management standards, making it easier for developers to use InstantSearch.js in various setups without manual modifications.

Package version

instantsearch.js 4.75.6

Operating system

No response

Browser

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@kevin-riveros kevin-riveros added the triage Issues to be categorized by the team label Jan 6, 2025
@Haroenv
Copy link
Contributor

Haroenv commented Jan 7, 2025

Sorry, this can not be done without a breaking change, or we would have to list out every single file (with and without extension).

It sounds like you're actually expecting cjs in both cases (jest and ssr), but actually authoring esm. Have you tried importing the cjs instead?

However we are working on a next major version, where we'll be able to simplify the exports and avoid this issue.

@jlowcs
Copy link

jlowcs commented Jan 7, 2025

Sorry, this can not be done without a breaking change, or we would have to list out every single file (with and without extension).

I don't think you would have to. Afaik, you'd only have to list the modules that people might want to access directly. The list that Kevin provided should be enough.

Edit: I got it, you mean that you're concerned that some apps in the wild are importing those modules by their real path, and adding those "exports" would break those builds.

It sounds like you're actually expecting cjs in both cases (jest and ssr), but actually authoring esm. Have you tried importing the cjs instead?

The issue is that we would still be importing instantsearch.js in other places which would use the ESM version in our webpack build. So that would end up being inconsistent.

@jlowcs
Copy link

jlowcs commented Jan 7, 2025

I think an alternative would be to add folders with a dedicated package.json. e.g. routers/packages.json which would have its own main/module/types properties pointing to ../cjs/ etc.

If our app were to import those folders (e.g. 'instantsearch.js/routers'), it would then try to resolve them by looking at that new routers/package.json file.

It should also not break any existing app that would be importing the files directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage Issues to be categorized by the team
Projects
None yet
Development

No branches or pull requests

3 participants