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

Conditional exports in resolve@next #209

Closed
coreyfarrell opened this issue Jan 25, 2020 · 3 comments
Closed

Conditional exports in resolve@next #209

coreyfarrell opened this issue Jan 25, 2020 · 3 comments

Comments

@coreyfarrell
Copy link

It's nice that the next version provides an ES module wrapper using conditional exports. One suggestion could you drop the require condition? The following seems to work:

".": {
  "import": "./index.mjs",
  "default": "./index.js"
}

The big difference is that require('resolve') will not print ExperimentalWarning: Conditional exports is an experimental feature. as default is not part of the conditional exports feature where require is. import('resolve') will produce the warning but that seems reasonable (hopefully this warning will not stay for long).

Also for my own information can you explain why you list sub-paths as:

		"./core": [
			{
				"default": "./lib/core.js"
			},
			"./lib/core.js"
		],

Couldn't this be reduced to "./core": "./lib/core.js"?

@ljharb
Copy link
Member

ljharb commented Jan 25, 2020

Dropping the require condition makes sense, yes.

The reason that the array is always required is that node v13.0 and v13.1 have a bug where although ESM is flagged, "exports" is still read, and they do not understand the object syntax. Thus, the arrays will have to be used in every package i maintain forever, including this one.

@coreyfarrell
Copy link
Author

Wouldn't 13.0 / 13.1 understand:

"exports": {
  ".": [
    {
      "import": "./index.mjs",
      "default": "./index.js"
    },
    "./index.js"
  ],
  "./core": "./lib/core.js"
}

I understand why you need the array notation for the main export which provides the ES module wrapper but for the sub-path remaps which do not have conditional exports I'm not sure why you need object notation at all?

@ljharb
Copy link
Member

ljharb commented Jan 25, 2020

If i want the subpaths to work in node 13.0 and 13.1, they also need the array form.

@ljharb ljharb closed this as completed in 6c25208 Jan 25, 2020
ljharb added a commit that referenced this issue Oct 21, 2020
Changes since v2.0.0-next.1:

 - [Breaking] remove `core`/`isCore` in favor of `is-core-module` package
 - [Fix] drop the "require" condition, since that causes an "experimental" warning to be printed (#209)

Including all changes in v1.15.1 - v1.18.1:

v1.18.1

 - [Fix] `core`: remove console warning on require, since the main entry point requires it
 - [Refactor] avoid using extensions in require paths
 - [meta] update auto-generated `core.json`
 - [meta] add `eclint` and `.editorconfig`
 - [Dev Deps] update `eslint`
 - [Dev Deps] add `aud` in `posttest`

v1.18.0

 - [New] extract `isCore` implementation to `is-core-module`
 - [New] add new core modules that will be in node v15
 - [readme] soft-deprecate `resolve.isCore`
 - [Dev Deps] update `eslint`, `@ljharb/eslint-config`, `tape`

v1.17.0

 - [New] `sync`/`async`: add `realpath`/`realpathSync` options (#218)
 - [Dev Deps] update `tape`

v1.16.1

 - [patch] when a non-node `fs` is broken and lacks `realpath`/`realpathSync`, do not crash (#220)

v1.16.0

 - [New] `core`: `fs/promises` is a core module again in node 14+ (f6473e2)
 - [patch] `sync`/`async`: use native `realpath` if available to unwrap symlinks (#217)

v1.15.1

 - [Fix] correct behavior when requiring `.` with same name (#212)
 - [Dev Deps] update `@ljharb/eslint-config`
 - [Tests] allow node 5 on windows to fail due to npm registry bug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants