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: allow overriding entry point in browser field #101

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

achingbrain
Copy link

Some modules are published with no main field in package.json and no /index.js, instead using exports to define the entry point for the module.

They sometimes specify an entry point for the browser using a "." or "./" key in the browser field, but currently these are broken with browserify as it tries to resolve /index.js when no main field is present.

The change here is to allow define the entry point for the module using the browser field:

{
  "browser": {
    ".": "path/to/index.js"
  }
}

Some modules are [published](https://unpkg.com/cborg@1.1.2/package.json) with no `main`
field in `package.json` and no `/index.js`, instead using `exports` to define the entry
point for the module.

They sometimes specify an entry point for the browser using a `"."` or `"./"` key in
the `browser` field, but currently these are broken with browserify as it tries to
resolve `/index.js` when no `main` field is present.

The change here is to allow define the entry point for the module using the `browser`
field:

```json
{
  "browser": {
    ".": "path/to/index.js"
  }
}
```
@ljharb
Copy link
Member

ljharb commented Mar 24, 2021

Can you elaborate on this?

Currently, resolve has zero support for the exports field (see browserify/resolve#224), and until that time, a package that lacks a main entirely (and also lacks an index.js) simply shouldn't be resolveable. (that package should, for back compat reasons, continue publishing a main for the foreseeable future: i see you've filed rvagg/cborg#9).

The "browser" field explicitly can't work with the "exports" field, so for this package - which doesn't actually provide a proper "browser" implementation - there's really no solution. The package links to./esm/cborg.js in its "browser" condition in the "exports" field, which is certainly what this package would select, but the implementation must be CJS, not ESM, for this package (and anything that expects the browser field) to support it.

@ljharb
Copy link
Member

ljharb commented Mar 24, 2021

In other words, the browser field spec does not permit a key of "." in this case, because that would node-resolve to the "main" or "index.js", both of which are missing here. The package you linked simply isn't compatible (yet) with the browserify ecosystem.

@rvagg
Copy link

rvagg commented Mar 25, 2021

welp, you may as well join this party @ljharb mikeal/ipjs#9
🤷 this all sucks, and I can't believe how much it still sucks after so much investment by so many people. At this stage I just want to know the path to most likely success without having to be stuck in 2014 forever.

@ljharb
Copy link
Member

ljharb commented Mar 25, 2021

I've subscribed to that issue, and I agree with mikeal's thoughts in the thread.

In my perspective, the only path to success is maintaining backwards compatibility. This means, either publishing packages as "only CJS" forever (which is fine, CJS and ESM are both first-class module systems in node, CJS is never going away, and isn't "legacy" in any way), or, publishing a dual package using one of these techniques:
(in all approaches, "main" and "exports" "default" point to CJS, and any file that's a singleton must always have precisely one CJS file that holds the source of truth for the state)

  1. author in CJS, hand-write thin ESM wrappers to provide named exports only where needed. "exports" "import" points to ESM.
  2. author in babel-flavored or node-native ESM, use babel to spit out CJS files, named exports come from these automatically, so you can publish only the CJS and then "exports" has no need for "import" - there's no value to publishing the ESM here.
  3. author in both node native ESM and CJS. "exports" "import" points to ESM. both are published (but note the singleton warning above)

In all these examples, a top-level "browser" field should point to CJS files that, when bundled by a non-broken module bundler, work in a browser. The "exports" field's "browser" condition should hold the same. If in the future a tool exists that can benefit from pointing to native ESM, and "import" is not sufficient, then a different "exports" condition would need to be invented to point to that - but since no such tool exists yet that I'm aware of, this isn't yet a use case that needs addressing.

achingbrain added a commit to Gozala/web-encoding that referenced this pull request Apr 6, 2021
> a top-level "browser" field should point to CJS files that, when bundled by a non-broken module bundler, work in a browser

browserify/browser-resolve#101 (comment)

I've changed the `browser` field to point to the `cjs` version of this module in
line with the above comment because this module is currently broken when used with
browserify.

I've also added the [util](https://www.npmjs.com/package/util) module as a dep since it's used
in the code. It would be ignored in most environments so the only cost is a slightly larger
bundle but it's likely to be included in any non-trivial bundle somewhere anyway so there's
likely to be no real-world impact.

Also:

- Adds typescript dep to tests that test typescript, otherwise tests fail with
  'cannot determine executable to run' - unless you have tsc installed globally I guess?
- Fixes a typo in a comment
- Simplifies .gitignore

Fixes #19
hugomrdias pushed a commit to Gozala/web-encoding that referenced this pull request Apr 6, 2021
> a top-level "browser" field should point to CJS files that, when bundled by a non-broken module bundler, work in a browser

browserify/browser-resolve#101 (comment)

I've changed the `browser` field to point to the `cjs` version of this module in
line with the above comment because this module is currently broken when used with
browserify.

I've also added the [util](https://www.npmjs.com/package/util) module as a dep since it's used
in the code. It would be ignored in most environments so the only cost is a slightly larger
bundle but it's likely to be included in any non-trivial bundle somewhere anyway so there's
likely to be no real-world impact.

Also:

- Adds typescript dep to tests that test typescript, otherwise tests fail with
  'cannot determine executable to run' - unless you have tsc installed globally I guess?
- Fixes a typo in a comment
- Simplifies .gitignore

Fixes #19
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.

3 participants