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

Do not add default node, browser, require or import conditions #12

Closed
SimenB opened this issue Oct 12, 2021 · 25 comments · Fixed by #13
Closed

Do not add default node, browser, require or import conditions #12

SimenB opened this issue Oct 12, 2021 · 25 comments · Fixed by #13

Comments

@SimenB
Copy link
Contributor

SimenB commented Oct 12, 2021

I'm looking into using this module to implement support for exports in Jest.

Seems unfortunate that Node's defaults should be forced on everybody using exports field. For a module that isn't supported to run in Node, it should just fail to resolve or something (or falling back to default) not pick up node.

There's already a switch in this library for browser condition so it's not consistently enforced (at least from my understanding browser is just a thing Node acknowledges exists, nowhere in the spec does it say its presence removes the node condition) - why not just fully let the caller provide their own conditions (barring possibly default - that seems reasonable to always add)? I.e. remove the two allows.add calls (and maybe the options leaving only conditions). Or possibly add node if no conditions is passed, but otherwise add nothing.

As a datapoint, TypeScript could not have used resolve.exports to implement support for types condition with their official example: https://devblogs.microsoft.com/typescript/announcing-typescript-4-5-beta/#packagejson-exports-imports-and-self-referencing since this module would have resolved import first.

I think resolve.exports should be implementation/host independent (maybe a resolve.exports-node or just a import { node } from 'resolve.exports' makes sense, but the base export shouldn't put these limitations there).

Tl;dr: removing

allows.add(require ? 'require' : 'import');
allows.add(browser ? 'browser' : 'node');
would allow this module to be used by any runtime/host to implement exports support without getting Node semantics/conditions when the module might not even run in Node or a browser.


This is mostly a copy paste of yarnpkg/berry#2431 (comment).

@lukeed
Copy link
Owner

lukeed commented Oct 12, 2021

Hey, sorry I missed you over in the PR! I kept clicking the notification, but GitHub never showed me anything except the top of the PR file, so I assumed it was was previous mentions from the other PR(s) that got merged into that one.

Ok, so I'm willing to work with you on this to see if there's something that can be done here, but there are a few things/misunderstands to correct first before we can figure out any next steps:

  1. This resolve.exports is meant to mimic Node's resolution algorithm. It applies the same defaults (node, default) and the same mutually exclusive constraints (import vs require) that Node enforces (and is required by the spec). I've added a browser option to opt into the browser condition, and as a convenience, it disables the node condition to ensure you people don't get node exports inside their browser builds.

  2. As a datapoint, TypeScript could not have used

    Their example is simply wrong – at least according to the spec, which you would hope/imagine they'd want to support since they're integrating with it... As written, this types condition is supposed to never be resolved. It's really quite unfortunate. For a recap, the resolution algorithm is this:

    • "default" – must always exist

    • one of "import" or "require" must always exist

    • "node" - matches for any Node.js environment + official endorsement of "browser" allowed me to justify toggling node vs browser condition

    • And, most importantly:

      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.
      Node.js Documentation

      So, they have "types" last. That means it's last priority and will/should never resolve because one of "import" or "require" will be defined and, as written, is a higher priority than "types". Most people forget about the key-order importance, but it's the only tool package authors have at their disposal for offering/swapping out module content based on requestor's preferences.

      Their example is effectively the equivalent of this:

      "exports": {
        ".": {
          "default": "./default/index.js",
          "types": "./types/index.d.ts"
        }
      }

      ...which (hopefully) no one argues that types should still be found in this case. If they do, they need to read the spec/algorithm definition once again.

      Additionally, they should end by reading this Resolving user conditions section, which proves that adding --conditions is additive and does not replace or modify the default conditions set.


Overall, the entire "exports" rollout/situation is (still) very confusing and big players like Jest/TypeScript should do their best to understand it and conform to the designs that were laid out, otherwise exports will become increasingly more confusing and will effectively become a free-for-all.

Resolving exports is a two-way street – package authors play an equally-important role as the resolving/consuming side. No one knows the package better than the author(s), so it's up to them to define the priority list. I like to think of it as a recommendation system, where the author(s) have all the products sorted by most to least specific. Consumer walks in and says "I like browser, worker, production, and import." and the author/sommelier says "Perfect! I have just the thing for you: ./worker/index.prod.mjs"

"exports": {
  "worker": {
    "import": {
      "production": "./worker/index.prod.mjs",
      "development": "./worker/index.dev.mjs",
      "default": "./worker/index.prod.mjs"
    },
    "require": {
      "production": "./worker/index.prod.js",
      "development": "./worker/index.dev.js",
      "default": "./worker/index.prod.js"
    }
  },
  "browser": {
    "import": "./browser/index.mjs",
    "require": "./browser/index.js"
  },
  // ...
}

@SimenB
Copy link
Contributor Author

SimenB commented Oct 12, 2021

Hi! Thanks for the response. I'm currently on the move so I just skimmed through, but this jumped out

"default" – must always exist

Even node's examples doesn't have this, so I'm not sure this is true? This is the first example in the docs: https://nodejs.org/api/packages.html#packages_conditional_exports

// package.json
{
  "main": "./main-require.cjs",
  "exports": {
    "import": "./main-module.js",
    "require": "./main-require.cjs"
  },
  "type": "module"
}

Might just be to keep the examples succinct, I dunno. But if this is a requirement by the spec we should probably open up an issue with node for them to fix their examples.

Overall, the entire "exports" rollout/situation is (still) very confusing and big players like Jest/TypeScript should do their best to understand it and conform to the designs that were laid out, otherwise exports will become increasingly more confusing and will effectively become a free-for-all.

I agree 100%! Which is why I wanna utilize a module written strictly to the spec rather than implementing our own which sorta works but misses all the edge cases and ends up behaving wrong when running in a "real" env 🙂


I'll read through you response thoroughly tomorrow and properly digest it 👍

@ljharb
Copy link

ljharb commented Oct 12, 2021

Indeed, default must not always exist. node's docs are that way to try to obscure all the things that the authors don't want you using, but all of my packages use default.

For require, i believe the first condition it finds among node/require/default will be used; for import, import/default?

@lukeed
Copy link
Owner

lukeed commented Oct 12, 2021

According to the PACKAGE_TARGET_RESOLVE spec, "default" is always looked at/considered alongside the actual conditions set. To my understanding, that is specifically to address cases like the worker definition above, because otherwise it'd be extra silly to have something like this instead:

"worker": {
    "import": {
      "production": "./worker/index.prod.mjs",
      "development": "./worker/index.dev.mjs",
      "import": "./worker/index.prod.mjs" // <- echo "import" instead of "default"
    },
    // ...
}

See Resolver Algorithm Specification, under PACKAGE_TARGET_RESOLVE here:

2. Otherwise, if target is a non-null Object, then
    1. If exports contains any index property keys, as defined in ECMA-262 6.1.7 Array Index, throw an Invalid Package Configuration error.
    2. For each property p of target, in object insertion order as,
        1. **If p equals "default"** or conditions contains an entry for p, then
            1. Let targetValue be the value of the p property in target.
            2. Let resolved be the result of PACKAGE_TARGET_RESOLVE( packageURL, targetValue, subpath, pattern, internal, conditions).
            3. If resolved is equal to undefined, continue the loop.
            4. Return resolved.

@SimenB: Even node's examples doesn't have this, so I'm not sure this is true?

That's because the example has import and require conditions defined, and only of them will always be active when resolving. For example, modifying the example to this is entirely redundant:

// package.json
{
  "main": "./main-require.cjs",
  "exports": {
    "import": "./main-module.js",
    "require": "./main-require.cjs",
    "default": "./main-module.js" // < ADDED: but will NEVER match
  },
  "type": "module"
}

@ljharb: i believe the first condition it finds among node/require/default will be used; for import, import/default?

This is true for all conditions. user conditions are a set of options, and the key order specified in the "exports" object determines the order that the resolver checks/determines a match. This is the bare minimum implementation of how "exports" conditions are traversed and nested:

// NOTE: `level` is only for logging
function resolve(input, conditions, level = 0) {
  let allows = new Set(conditions);
  allows.add('default');

  let spacer = ' '.repeat(level * 4);

  for (let key in input) {
    console.log('%s [%s] look', spacer, key);

    if (allows.has(key)) {
      console.log('%s [%s] KNOWN', spacer, key);
      let tmp = input[key];
      let type = typeof tmp;
      if (type === 'string') {
        console.log('%s [%s] DONE', spacer, key, tmp);
        return tmp;
      }

      if (tmp && type === 'object') {
        console.log('%s [%s] NESTED', spacer, key);
        return resolve(tmp, conditions, level + 1);
      }

      return console.log('%s [%s] INVALID VALUE', spacer, key, tmp);
    } else {
      console.log('%s [%s] ignore', spacer, key);
    }
  }
}

It's a simple for-in loop of the exports object, and follows conditions recursively if they're objects.
Now, obviously this implementation is incomplete – and untested – I just threw it together here for demonstration purposes.

Here are some examples (warning: wall of text):

// Example "exports" object
// All `resolve` examples will use this as input:
let example = {
  worker: {
    import: "./worker.mjs",
    require: "./worker.js"
  },
  browser: {
    import: {
      production: "./browser.prod.mjs",
      development: "./browser.dev.mjs",
      default: "./browser.prod.mjs"
    },
    require: {
      production: "./browser.prod.js",
      development: "./browser.dev.js",
      default: "./browser.prod.js"
    }
  },
  import: "./fallback.mjs",
  require: "./fallback.js"
};

// Find file when supporting "browser" and "import" conditions
// ... and default, implicitly/by default
resolve(example, ['browser', 'import']);
//=> [worker] look
//=> [worker] ignore
//=> [browser] look
//=> [browser] KNOWN
//=> [browser] NESTED
//=> 		[import] look
//=> 		[import] KNOWN
//=> 		[import] NESTED
//=> 				[production] look
//=> 				[production] ignore
//=> 				[development] look
//=> 				[development] ignore
//=> 				[default] look
//=> 				[default] KNOWN
//=> 				[default] DONE ./browser.prod.mjs


resolve(example, ['browser', 'development', 'import']);
//=> [worker] look
//=> [worker] ignore
//=> [browser] look
//=> [browser] KNOWN
//=> [browser] NESTED
//=> 		[import] look
//=> 		[import] KNOWN
//=> 		[import] NESTED
//=> 				[production] look
//=> 				[production] ignore
//=> 				[development] look
//=> 				[development] KNOWN
//=> 				[development] DONE ./browser.dev.mjs


resolve(example, ['worker', 'require']);
//=> [worker] look
//=> [worker] KNOWN
//=> [worker] NESTED
//=> 		[import] look
//=> 		[import] ignore
//=> 		[require] look
//=> 		[require] KNOWN
//=> 		[require] DONE ./worker.js


resolve(example, ['require']);
//=> [worker] look
//=> [worker] ignore
//=> [browser] look
//=> [browser] ignore
//=> [import] look
//=> [import] ignore
//=> [require] look
//=> [require] KNOWN
//=> [require] DONE ./fallback.js


resolve(example, ['foobar']);
//=> [worker] look
//=> [worker] ignore
//=> [browser] look
//=> [browser] ignore
//=> [import] look
//=> [import] ignore
//=> [require] look
//=> [require] ignore
// (returns undefined)

I have a lot of tests in this repo that are hopefully intelligible to fresh eyes. The core approach is correct and the default conditions match Node's. The algorithm actually works quite well and is very flexible/accommodating, but it does require authors to play their part too. It's the only way we can dream to have a multi-environment resolution standard, which ESM allows & is begging for.

@ljharb
Copy link

ljharb commented Oct 12, 2021

Note that the import/require/default example will match default in node versions that lack the import and require conditions.

@lukeed
Copy link
Owner

lukeed commented Oct 12, 2021

No because import is always enabled when importing and require is always enabled when requireing. There is no other avenue into a module, so one of those is always on. A "default" can be matched but only if it's positioned before/above other conditions.

@ljharb
Copy link

ljharb commented Oct 12, 2021

@lukeed that's not the case in older versions of node, which don't understand require or import or node at all. Check in node v13.2-v13.6, for example.

@SimenB
Copy link
Contributor Author

SimenB commented Oct 12, 2021

That's the case in this lib (and in Node) but should it be true for exports in package.json general? If yes, I think the usability of the field is less than it could (should) be, but fine if it exists solely for Node.js. But then it's not usable for modules distributed on npm that's not meant for Node, which seems like a solution where everybody loses?

Require.js, system.js, amd, umd etc are all (imo) valid targets for an exports, even if node doesn't support them (and have their own defaults).

To circle back to the OP - I don't necessarily have any objections to this module being an implementation of Node's algorithm, but in that case it should be clear that's the case (and that exports is only for use in node and nowhere else) and not a more generic "entry points in JavaScript package distributed on npm (or its ilk)". And given Node took the time to write a spec, it seems it's meant to target more broadly than only node's own use cases?

(Written on mobile, please excuse terseness/typos)

@lukeed
Copy link
Owner

lukeed commented Oct 12, 2021

@ljharb and those versions are early iterations and no longer the resolution algorithm. They've been thrown out. They're also non-LTS versions of Node, which should not be built to or catered towards. And this version range, specifically, has caused so much adoption headache. I personally wish they'd see patch fixes that conformed them to the finalized behavior. It's a wound that's still open. In fact, I see a lot of people specifying this version range in their engines.node value: "^12.20.0 || >=14" specifically cuz of 13 brokenness.

@SimenB If tools want adopt the exports practice, then they should follow its design or make its own thing. Not ruin/muddy an existing design. The Node resolution algorithm is adequately flexible and can handle all use cases -- I was slightly skeptical of it at first too but it's proven to be a very valuable feature when publishing packages for use across Node, browsers, and Cloudflare Workers. It's been extremely successful for well over a year now.

And you're right, the time was taken to design a flexible system (thank you Node 13-13.7 for the early failures). The Node-isms are very very minimal and already, as part of the design, has an easy way to define and prioritize entries ahead of what Node would look for.

@SimenB
Copy link
Contributor Author

SimenB commented Oct 12, 2021

@SimenB If tools want adopt the exports practice, then they should follow its design or make its own thing.

I agree, but I would hope that exports targets wider than just node (like browser, deno, electron). Again, totally fair if it doesn't, but then its value is way lower than it could be (and it should be explicit in docs that this is the case)

@lukeed
Copy link
Owner

lukeed commented Oct 12, 2021

I'm saying it does target non-node environments. That's the whole purpose of user conditions. The only thing to be aware of, as an author, is that the order of your defined conditions determines the priority of the condition itself. I, personally, have used the same module across browser, workers, node, and electron – each with their own entry files – and all through defining the conditions in my package. It's very well-designed, it only suffers from a lack of education / clarity.

@ljharb
Copy link

ljharb commented Oct 12, 2021

@lukeed i absolutely agree they should all get patch fixes, but node has declined to do so. whether they "should" be supported is each author's choice; every package i maintain will continue to work with every possible node minor for as long as I maintain them :-)

It sounds like as long as a browser condition goes first, say, then a tool looking for the browser entrypoint will get it?

@lukeed
Copy link
Owner

lukeed commented Oct 12, 2021

Node 13 is a dead version – and literally no one has any reason to continue using it. I do not say this as live-on-the-cutting-edge type of person. I generally hold onto support for LTS versions of Node longer than the average developer, but odd-number versions are purposefully experimental. Node itself does not recommend that any one use odd versions in production or beyond the next even-version's release. I'm sure you're familiar with this, so I'm not sure why you are trying to advocate for this idea? Even Node 14 enters "maintenance" in 7 days. Anyone on Node 9/11/13 deserves a bad experience at this point.

It sounds like as long as a browser condition goes first, say, then a tool looking for the browser entrypoint will get it?

Yes, I've laid out a number of examples explaining this. If you only have time to skim one, I'd recommend you look at the output for the resolve(example, ['browser', 'development', 'import']); case above.

The exports key order defines the priority.
The resolving conditions defines keys the consumer is willing to accept, if defined.

@lukeed
Copy link
Owner

lukeed commented Oct 14, 2021

In a private conversation, it was confirmed to me that TypeScript is knowingly not conforming to the spec (currently, maybe there's still hope?) and are not considering key-order in their resolution. I very much disagree with this but who am I to say

@ljharb
Copy link

ljharb commented Oct 14, 2021

Did you get any info (links, preferably) as to why?

@lukeed
Copy link
Owner

lukeed commented Oct 14, 2021

No, just that TS already adjusts/ignores specs when loading anything (eg, .ts preference over .js) so that this isn't outside its wheelhouse. That's not a good reason to me -- and overall this move is going to add further confusion wrt "exports" and how it works/is supposed to be. A bit frustrating since the example/approach would work perfectly fine -- even with their non-compliant loader -- if it were this:

"exports": {
  "types": "./types/index.d.ts",
  "import": "./esm/index.js",
  "require": "./commonjs/index.cjs"
}

Because then it'd actually work with spec-compliant resolvers, and if they chose to continue ignoring the spec within their own resolver, their resolver would still work too.

Any spec-compliant revolver that wasn't looking for "types" condition would safely ignore it.

@SimenB
Copy link
Contributor Author

SimenB commented Oct 14, 2021

I'm saying it does target non-node environments. That's the whole purpose of user conditions. The only thing to be aware of, as an author, is that the order of your defined conditions determines the priority of the condition itself. I, personally, have used the same module across browser, workers, node, and electron – each with their own entry files – and all through defining the conditions in my package. It's very well-designed, it only suffers from a lack of education / clarity.

Right, I think I'm starting to see where you're coming from. I'm not saying that the way this module works today makes integrating into other runtimes/hosts/resolvers impossible, it just places the burden onto package authors whose packages are used in these envs to always put require/import last rather than the authors or users of said runtime.

And even if those conditions were put last, if my runtime doesn't support require or import (and isn't browser or node), but defines its own condition with a custom module system and I then load a module who inly defines those, I'd expect it to fall back to attempt to load the conditions I define (plus default probably). If I were to use this module to do resolving of exports, I would get one of those conditions I don't support back (with no way of knowing which condition was used as the only a path is returned). In this case I'd expect to get default (and an error if default is not included in the package).

I wouldn't expect package authors to support my custom runtime, but I would then want to surface that to my users ("package foo doesn't support this runtime") rather than fail at runtime with a syntax error or a undefined is not a function.


Anyways, to be less abstract, I can focus on my own use case, which is Jest.

The current approach in this module probably works, since the environments we do support are

  • node (ish, from my understanding this might return import _or require since it's "anything node can load"?)
  • browser (via JSDOM)
  • import (if ESM is enabled)
  • require (even if ESM is enabled since it can load both)

It's sorta weird that if I pass options.conditions: ['require'] you'll still add import since options.require isn't set, but that's easy enough to work around via a require: conditions.includes('require') from my own code.


So to wrap up - I have nothing against 100% respecting key order (and I find it weird TS pretends they don't matter), my only issue is default conditions I might not support being resolved (which I guess would be undefined behavior based on the runtime its loaded in) instead of "this package doesn't support any of your conditions".

@lukeed
Copy link
Owner

lukeed commented Oct 14, 2021

it just places the burden onto package authors whose packages are used in these envs to always put require/import last

As mentioned, this is a two-way street. Package authors are required to have some awareness of their exports definition.

But you're right, more often than not you'll be defining the import and require conditions last in the object and defining environment-specific conditions first. This is why key-order matters and also why nested conditions are allowed:

exports: {
  env1: {
    import: ..
    require: ...
  },
  env2: {
    // ...
  },
  import: ...
  require: ...
}

Here's probably my most complex public example; httpie@next. When importing the root module, a resolver aware of the "worker" condition will always get that code, else browser (if wanted), else it's Node with import-type used as differentiator. I could have had nested import/require conditions within "worker" and "browser", but as an author I'm aware of the fact that users will be using bundlers in worker/browser environments, meaning I only want to offer them esm anyway.

And even if those conditions were put last, if my runtime doesn't support require or import (and isn't browser or node), but defines its own condition with a custom module system and I then load a module who inly defines those, I'd expect it to fall back to attempt to load the conditions I define (plus default probably). If I were to use this module to do resolving of exports, I would get one of those conditions I don't support back

You'd only ever get a result/match for something you said you supported and/or wanted. Aka, in the httpie example, no one will ever get the "worker" file unless it was explicitly allowed. That's the purpose/safety of custom conditions -- author offers standard X conditions but also offers custom, optional Y conditions (worker, react-native, electron... cough types cough lol) that relevant users can opt into since theyre the target.

As an author, if you define this:

"exports": {
  ".": {
    "foobar": "./foobar.mjs"
  }
}

No one will ever see that file unless they've enabled the "foobar" condition (both with this module and with Node natively). Instead, the root import will resolve to nothing/undefined by default, which surfaces as undefined in this module and throws an Error in Node.

However, doing this:

"exports": {
  ".": {
    "foobar": "./foobar.mjs",
    "default": "./fallback.js"
  }
}

...will resolve to the "fallback.js" file by default for this module and Node, avoiding the undefined/Error.


If you really need me to (99.9999% you don't though), I can add an unsafe option that clears the allows conditions set so that there are no defaults.

I'd want to keep the name ugly/scary because it's purposefully breaking out of defined behavior. Again, this really should not be needed by anybody who's actually willing to use/work with the exports field.

@SimenB
Copy link
Contributor Author

SimenB commented Oct 14, 2021

As an author, if you define this:

"exports": {
  ".": {
    "foobar": "./foobar.mjs"
  }
}

No one will ever see that file unless they've enabled the "foobar" condition (both with this module and with Node natively). Instead, the root import will resolve to nothing/undefined by default, which surfaces as undefined in this module and throws an Error in Node.

However, doing this:

"exports": {
  ".": {
    "foobar": "./foobar.mjs",
    "default": "./fallback.js"
  }
}

...will resolve to the "fallback.js" file by default for this module and Node, avoiding the undefined/Error.

I'm very much down with a default condition always resolving if no other conditions match - the case I'm worried about is

"exports": {
  ".": {
    "foobar": "./foobar.mjs",
    "node": "./node.mjs",
    "require": "./common.cjs",
    "import": "./import.mjs",
    "default": "./fallback.js",
  }
}

that would never give me default, right? Somewhat convoluted example (there's probably only ever gonna be node and possibly import, not node and require, but who knows).

Or if I were to remove the default condition from the example - I would want some way of then still getting undefined back

@lukeed
Copy link
Owner

lukeed commented Oct 14, 2021

According to the spec, if a "default" condition is defined, you will never get undefined.

In a Node setting, (options.browser remains falsely) then you will always get "node" to match. That's because the author defined it that way. Because it's evaluated before "require"/"import" -- and matches -- then that means this "node" value will always be used, regardless of require("example") or import("example")

Now, if it's a browser environment (options.browser = true), then "node" is disabled internally and the resolver is finally able to move beyond that "node" condition.

If options.require were enabled, then that means the "require" condition is enabled, else "import" is enabled. That determines which of the next two keys in your example will match.

If, for some reason, you pass options.conditions an array such that both "require" and "import" are enabled (this should never happen, it's supposed to be determined by the usage type), then your example will always return "./common.cjs" for browser settings since "require" is defined/evaluated before "import".

So ya, you'll never get to the "default" key in this example (according to spec) because you're ONLY able to access a file thru import or require which means one of those conditions will always match & that match happens before "default" is ever looked at.

Sorry for long-winded answer but hopefully that helps
(Bedtime)

@SimenB
Copy link
Contributor Author

SimenB commented Oct 14, 2021

Ah good point, I forgot import and require are mutually exclusive (called out in https://nodejs.org/api/packages.html#packages_conditional_exports) so that shouldn't ever happen as conditions. 👍

So ya, you'll never get to the "default" key in this example (according to spec) because you're ONLY able to access a file thru import or require which means one of those conditions will always match & that match happens before "default" is ever looked at.

Right, and I think that's partly where my issue comes from - what if it's not import or require (e.g. require.js SystemJS etc.)? In Jest's case these don't matter (since those 2 are the only conditions we support), but I think a generic exports resolver should handle it? I can't see anywhere in Node's docs where they say "the spec is that these conditions are always provided".

https://nodejs.org/api/packages.html#packages_conditional_exports only says "node supports these conditions" and https://nodejs.org/api/esm.html#esm_resolver_algorithm talks specifically about how node resolves ESM (and which default conditions node provides), it doesn't say "this is the way all ESM should be resolved".


However, purely from Jest's perspective (which only has require and import), I think I'd be golden as long as node or browser was not forced on me. If a user has a custom test environment that simulates some JS environment that's not Node or the browser, I would always want this to resolve to default (or undefined if default wasn't in there)

"exports": {
  ".": {
    "node": "./node.mjs",
    "browser": "./browser.cjs",
    "default": "./fallback.js"
  }
}

Sorry for long-winded answer but hopefully that helps

It definitely does, thanks for being patient with me!

@lukeed
Copy link
Owner

lukeed commented Oct 14, 2021

@SimenB I gotta run, but I put up a quick PR that should address your case/concerns. Tests attached that also encompasses your last snippet.

When unsafe is on, the only other option that has an interplay with it is conditions – so the idea is that by using unsafe you are directly passing in the list conditions you want to support. Only default is left on as a default condition – I think you agreed earlier that there was no logical reason to remove this.

@SimenB
Copy link
Contributor Author

SimenB commented Oct 14, 2021

Thank you!

Only default is left on as a default condition – I think you agreed earlier that there was no logical reason to remove this.

Yup, I very much agree with this 🙂

@lukeed
Copy link
Owner

lukeed commented Oct 15, 2021

Released in 1.1.0~! Hopefully added enough warning labels for the general use case, but I guess time will tell 😆 Guess this means that TS could have used this now directly, although I'm sure they already have their recursive if ("types" in thing) checker in place.

@SimenB
Copy link
Contributor Author

SimenB commented Oct 15, 2021

Thank you!

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.

3 participants