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

"main" and "browser" root fields in package.json #9

Open
hugomrdias opened this issue Mar 15, 2021 · 11 comments
Open

"main" and "browser" root fields in package.json #9

hugomrdias opened this issue Mar 15, 2021 · 11 comments

Comments

@hugomrdias
Copy link

while debugging and build issue with @ipld/car i noticed that the browser field can't be resolved by esbuild

 "browser": {
    ".": "./cjs/car-browser.js",
    "./reader": "./cjs/lib/reader-browser.js",
    "./indexed-reader": "./cjs/lib/indexed-reader-browser.js",
    "./indexer": "./cjs/lib/indexer.js",
    "./iterator": "./cjs/lib/iterator.js",
    "./writer": "./cjs/lib/writer.js"
  },

i think this is trying to mimic how export maps work with abstract entry points to the package but the old main and browser fields dont care about that, they just care about "real" files and mapping "real" files to other "real" files.
In the snippet above we can see that "." or "./reader" etc don't actually exist inside @ipld/car and so esbuild can't bundle and errors saying cant resolve @ipld/car.

A fix would be to output this instead

 "main": "./cjs/car.js",
 "browser": {
    "./cjs/car.js": "./cjs/car-browser.js",
    "./cjs/lib/reader.js": "./cjs/lib/reader-browser.js",
    "./cjs/lib/indexed-reader": "./cjs/lib/indexed-reader-browser.js",
    "./cjs/lib/indexer": "./cjs/lib/indexer.js", // optional
    "./cjs/lib/iterator": "./cjs/lib/iterator.js", // optional
    "./cjs/lib/writer": "./cjs/lib/writer.js" // optional
  },

Theres other ways to fix this but this seems to be most canonical one.

@mikeal
Copy link
Owner

mikeal commented Mar 15, 2021

@rvagg added a feature flag for this but the latest esbuild supports export maps now, would be best to upgrade.

@hugomrdias
Copy link
Author

we updated esbuild but we still see these issues. i still need to research more about what is happening here but independent from esbuild if ipjs outputs the browser field like this, it would be useful to do it in a more canonical way.

I'm guessing this browser field is for bundlers that dont support exports ? im confused about the use case here, because the root browser takes the bundler to the cjs code and the exports browser to the esm code.

@rvagg
Copy link
Collaborator

rvagg commented Mar 16, 2021

I'm guessing this browser field is for bundlers that don't support exports?

It's more complicated than that unfortunately. I think js-multiformats is the best example because it has to do things very differently in the browser where implementations used are entirely different (although @ipld/car does try and do a similar thing).

https://unpkg.com/browse/multiformats@4.6.1/package.json

This works with WebPack, but of course that's just one of many and they all seem to have different opinions. I don't know what the majority opinion is on how to use "browser" but we're going with an apparent majority opinion here and my initial guess would be that esbuild is in the minority of assuming that it's a mapping of actual files to browser files rather than a mapping of imports/requires to browser files.

We could switch "main" back on now by compiling with --main but it'd be nice to avoid that if we can so we don't break other things (TypeScript being the one that broke when we first tried it! although that's apparently fixable). Modifying how "browser" works though is another matter because it will likely result in breakage in other bundlers (WebPack the known one but I suspect browserify is the same).

@mikeal
Copy link
Owner

mikeal commented Mar 16, 2021

There’s a spec for the browser field https://github.com/defunctzombie/package-browser-field-spec

TBH, esbuild is doing some good stuff but the enthusiasm for its performance is a bit premature since it hasn’t actually met the feature set of other build tools. Everywhere I’ve seen it adopted I’ve also seen new problems, many of which have not yet been resolved. It’s promising but still immature.

@evanw
Copy link

evanw commented Mar 17, 2021

There’s a spec for the browser field https://github.com/defunctzombie/package-browser-field-spec

I'm the author of esbuild. Unfortunately that spec for the browser field you linked doesn't really specify much, is unmaintained, and comes without any tests, so has been pretty much impossible for me to know what the behavior should be without getting bug reports about cases that don't work. Please feel free to reach out if something isn't working. I'm happy to fix path resolution in esbuild if something is wrong.

I just tried giving this a go and it looks like using the browser field to remap "." doesn't work with Webpack or Browserify either:

Click to see the source code
{
  "dependencies": {
    "@ipld/car": "0.1.2",
    "browserify": "17.0.0",
    "esbuild": "0.9.2",
    "webpack": "5.26.2",
    "webpack-cli": "4.5.0"
  }
}
const fs = require('fs')

// Make sure we're testing "browser" not "exports"
const jsonPath = __dirname + '/node_modules/@ipld/car/package.json'
const json = JSON.parse(fs.readFileSync(jsonPath, 'utf8'))
delete json.exports
fs.writeFileSync(jsonPath, JSON.stringify(json, null, 2))

// Check multiple import paths
const tests = [
  '@ipld/car',
  '@ipld/car/indexed-reader',
]

async function main() {
  const entryPath = __dirname + '/entry.js'

  // Try esbuild
  console.log('\nesbuild:')
  for (const test of tests) {
    fs.writeFileSync(entryPath, `require(${JSON.stringify(test)})`)
    try {
      const esbuild = require('esbuild')
      await esbuild.build({
        entryPoints: [entryPath],
        bundle: true,
        write: false,
        logLevel: 'silent',
      })
      console.log(`✅ ${test}`)
    } catch (e) {
      console.log(`🚫 ${test}\n${e.message}`.replace(/\n/g, '\n  '))
    }
  }

  // Try webpack
  console.log('\nwebpack:')
  for (const test of tests) {
    fs.writeFileSync(entryPath, `require(${JSON.stringify(test)})`)
    try {
      const webpack = require('webpack')
      await new Promise((resolve, reject) => webpack({
        mode: 'development',
        entry: entryPath,
        devtool: 'hidden-source-map',
      }, (err, stats) => {
        if (err) reject(err);
        if (stats.hasErrors()) reject(stats.toString());
        else resolve();
      }));
      console.log(`✅ ${test}`)
    } catch (e) {
      console.log(`🚫 ${test}\n${e.split('\n').slice(3, 5).join('\n')}`.replace(/\n/g, '\n  '))
    }
  }

  // Try browserify
  console.log('\nbrowserify:')
  for (const test of tests) {
    fs.writeFileSync(entryPath, `require(${JSON.stringify(test)})`)
    try {
      const browserify = require('browserify')
      const result = await new Promise((resolve, reject) => {
        browserify(entryPath).bundle((err, data) => {
          if (err) reject(err)
          else resolve(data)
        })
      })
      console.log(`✅ ${test}`)
    } catch (e) {
      console.log(`🚫 ${test}\n${e}`.replace(/\n\s*/g, '\n  '))
    }
  }
}

main()
esbuild:
🚫 @ipld/car
  Build failed with 1 error:
  entry.js:1:8: error: Could not resolve "@ipld/car" (mark it as external to exclude it from the bundle)
✅ @ipld/car/indexed-reader

webpack:
🚫 @ipld/car
  ERROR in ./entry.js 1:0-20
  Module not found: Error: Can't resolve '@ipld/car' in '/Users/evan/Desktop/foo'
✅ @ipld/car/indexed-reader

browserify:
🚫 @ipld/car
  Error: Can't walk dependency graph: Cannot find module '@ipld/car' from '/Users/evan/Desktop/foo/entry.js'
  required by /Users/evan/Desktop/foo/entry.js
✅ @ipld/car/indexed-reader

With only the browser field and not the exports field, all three cannot resolve @ipld/car but can resolve @ipld/car/indexed-reader. So it looks like the way esbuild is handling this is consistent with other implementations of the browser field in this case?

@mikeal
Copy link
Owner

mikeal commented Mar 17, 2021

@defunctzombie @FredKSchott thoughts?

@FredKSchott
Copy link

FredKSchott commented Mar 17, 2021

I think Rollup does have some handling for a browser map like you have here (which esinstall/Snowpack/Skypack all take advantage of), but I agree with @evanw that that behavior has always felt undefined to me. imo the sooner you can move over to 100% export maps, the better.

(also: "browser" pointing to CJS instead of ESM? blasphemy! :)

@evanw
Copy link

evanw commented Mar 17, 2021

we updated esbuild but we still see these issues

This is a bug in esbuild btw. The bug caused export maps to not work with scoped packages in esbuild. I'll push out the fix for it soon. This isn't surprising since export maps is a brand new feature in esbuild and isn't battle-tested yet.

But even with the fix, the @ipld/car@0.1.2 package hits other issues when it's bundled with esbuild due to the use of built-in node modules in a browser build. I'm guessing this second-order problem is caused by the order of conditions in the exports map:

  "exports": {
    ".": {
      "require": "./cjs/car.js",
      "import": "./esm/car.js",
      "browser": "./esm/car-browser.js"
    },

If browser comes after require and import, then the browser export will never be used since all imports are either a require or an import and conditions are checked in the order they appear in package.json. You'd have to move browser before require and import if you want it to take effect.

@rvagg
Copy link
Collaborator

rvagg commented Mar 17, 2021

ugh

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.

Semantic map key ordering, wonderful idea.. I bet you have fun with that in Go @evanw.

We can get that order switched around here pretty easily I think, I think it would make sense to put "browser" at the top.

So re the top-level "browser", does anyone have any idea how close we are to being able to remove that entirely? Do all the widely-used current-generation bundlers all support "exports" now?

@hugomrdias
Copy link
Author

FYI I already released aegir with esbuild fix for scoped packages.

@evanw
Copy link

evanw commented Mar 18, 2021

So re the top-level "browser", does anyone have any idea how close we are to being able to remove that entirely? Do all the widely-used current-generation bundlers all support "exports" now?

I suspect Parcel doesn't support "exports" yet since this is still open: parcel-bundler/parcel#4155.

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

No branches or pull requests

5 participants