web-std-io icon indicating copy to clipboard operation
web-std-io copied to clipboard

fix: demote browser export

Open penx opened this issue 3 years ago • 2 comments

Prefer require, import and types exports over browser.

Exports are resolved in order, i.e. the first match is used.

I'm not sure why the browser export is using the raw src, however when Jest is configured with testEnvironment: "jsdom", it will resolve to this file causing 2 issues:

  • Jest will not transpile it unless added to transformIgnorePatterns or experimental ESM modules support is enabled
  • Jest throws a type error as discussed on https://github.com/remix-run/remix/issues/3402

Also:

"types" - can be used by typing systems to resolve the typing file for the given export. This condition should always be included first. https://nodejs.org/api/packages.html

penx avatar Jun 06 '22 20:06 penx

🦋 Changeset detected

Latest commit: 910500232627dc4c8017f0d371e381ebe3293ca7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@remix-run/web-fetch Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Jun 06 '22 20:06 changeset-bot[bot]

CC/ @chaance @jacob-ebey @mjackson Since I can't add you as a reviewer (I don't have any permissions on this repo), I wanted to tag you guys.

MichaelDeBoey avatar Jun 06 '22 21:06 MichaelDeBoey

I've been digging for over two hours and this is clearly the fix. Is there any progress on this PR? :innocent:

EDIT: Solving it with patch-package for now.

JRasmusBm avatar Nov 17 '22 08:11 JRasmusBm

nudge

nitrog7 avatar Jul 19 '23 03:07 nitrog7

Exports are resolved in order, i.e. the first match is used.

Is there a source/reference for this?

brophdawg11 avatar Jul 19 '23 17:07 brophdawg11

Exports are resolved in order, i.e. the first match is used.

Is there a source/reference for this?

Within the "exports" object, key order is significant. During condition matching, earlier entries have higher priority and take precedence over later entries. The general rule is that conditions should be from most specific to least specific in object order.

https://nodejs.org/api/packages.html#conditional-exports

penx avatar Jul 19 '23 17:07 penx

I don't believe this is "the fix" everyone is after here. Generally, any non-standard conditions like "browser" are put above the "require", "import", and "default" conditions. Reasoning being some bundlers (like esbuild) don't allow you to disable the default conditions, and instead configure conditions to check before the defaults.

This honestly sounds like a Jest issue of loading the wrong conditions for where they are executing. You aren't actually running Jest in the browser, so they should not be attempting to load the browser condition.

Moving browser below the standard conditions will mean that the "browser" condition will never be picked, even when building for / targeting the browser.

jacob-ebey avatar Jul 19 '23 18:07 jacob-ebey

Thanks @penx! I missed that when I looked through those docs.

I agree with @jacob-ebey here - that snippet sounds to me like browser belongs above import/require as it's more specific. Just like node should be above per the docs.

It does feel like Jest either (1) should not pick up the browser entry or (2) should handle ESM due to type:module.

I think we should split off the moving types to the top into a new Pr and merge that since that feels cut and dry. Moving browser around feels wrong and likely requires some more investigation into exactly why jest can't handle it correctly.

brophdawg11 avatar Jul 19 '23 18:07 brophdawg11

Extracted the lifting of types to the top into https://github.com/remix-run/web-std-io/pull/38

brophdawg11 avatar Aug 01 '23 20:08 brophdawg11

Instead of demoting the browser export, we should provide a CJS version for browser as well, which is exactly what I did in #43

MichaelDeBoey avatar Aug 22 '23 19:08 MichaelDeBoey