search

browserify / browser-resolve

resolve function which support the browser field in package.json
MIT License
102 stars 70 forks source link

Update resolve dependency to 1.12.0 #93

Closed pravi closed 4 years ago

pravi commented 5 years ago

The following tests fail with resolve 1.12.0

$ mocha -R spec

  ✓ shim found
  ✓ core shim not found
  ✓ shim found
  ✓ core shim not found
  ✓ module to implicit extension
  ✓ implicit extension to implicit extension
  ✓ implicit extension to implicit extension
  ✓ explicit extension to explicit extension
  ✓ implicit extension to explicit extension
  ✓ module implicit extension to explicit extension
  ✓ module implicit extension to explicit extension
  ✓ false file
  ✓ false module
  ✓ false file
  ✓ false module
  ✓ false expand path
  ✓ local
  ✓ local
  ✓ local
  ✓ index.js of module dir
  ✓ alternate main
  ✓ string browser field as main
  ✓ string browser field as main - require subfile
  ✓ object browser field as main
  ✓ object browser field replace file
  ✓ object browser field replace file - no paths
  ✓ replace module in browser field object
  ✓ index.js of module dir
  ✓ alternate main
  ✓ string browser field as main
  ✓ string browser field as main - require subfile
  ✓ object browser field as main
  ✓ object browser field as main
  ✓ object browser field replace file
  ✓ test foobar -> module-b replacement
  ✓ test ./x -> ./y replacement
  ✓ test core -> module-c replacement
  ✓ test foobar -> module-b replacement with transform
  ✓ test foobar -> module-i replacement with transform in replacement
  ✓ object browser field replace file - no paths
  ✓ replace module in browser field object
  ✓ override engine shim
  ✓ alt-browser field
  1) index.js of module dir
  ✓ alternate main
  ✓ string browser field as main
  ✓ string browser field as main - require subfile
  ✓ string alt browser field as main - require subfile
  ✓ object browser field as main
  ✓ object browser field as main
  2) deep module reference mapping
  3) deep module reference mapping without file extension - .js
  4) deep module reference mapping without file extension - .json
  ✓ object browser field replace file
  ✓ test foobar -> module-b replacement
  ✓ test core -> module-c replacement
  ✓ test core -> module-c replacement with alt browser
  ✓ test foobar -> module-b replacement with transform
  ✓ test ./x -> ./y replacement
  ✓ test foobar -> module-i replacement with transform in replacement
  ✓ object browser field replace file - no paths
  ✓ replace module in browser field object
  ✓ override engine shim
  ✓ alt-browser field
  5) alt-browser deep module reference mapping
  ✓ alt-browser fallback to "browser" on deps of deps
  6) not fail on accessing path name defined in Object.prototype

  61 passing (198ms)
  6 failing

  1) index.js of module dir:
     Uncaught AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
+ expected - actual

- {
-   main: 'fixtures'
- }
+ undefined
      at test/modules.js:11:16
      at index.js:269:13
      at node_modules/resolve/lib/async.js:93:25
      at maybeUnwrapSymlink (node_modules/resolve/lib/async.js:35:9)
      at node_modules/resolve/lib/async.js:89:24
      at ondir (node_modules/resolve/lib/async.js:264:27)
      at onex (node_modules/resolve/lib/async.js:161:32)
      at node_modules/resolve/lib/async.js:11:20
      at FSReqWrap.oncomplete (fs.js:155:5)

  2) deep module reference mapping:
     Uncaught AssertionError [ERR_ASSERTION]: ifError got unwanted exception: Cannot find module 'module-l/direct' from '/home/vagrant/forge/debian/git/js-team/node-browser-resolve/test/fixtures'
      at test/modules.js:95:16
      at index.js:265:24
      at node_modules/resolve/lib/async.js:99:17
      at node_modules/resolve/lib/async.js:97:35
      at processDirs (node_modules/resolve/lib/async.js:244:39)
      at isdir (node_modules/resolve/lib/async.js:251:32)
      at node_modules/resolve/lib/async.js:23:69
      at FSReqWrap.oncomplete (fs.js:154:21)

  3) deep module reference mapping without file extension - .js:
     Uncaught AssertionError [ERR_ASSERTION]: ifError got unwanted exception: Cannot find module 'module-n/foo' from '/home/vagrant/forge/debian/git/js-team/node-browser-resolve/test/fixtures'
      at test/modules.js:106:16
      at index.js:265:24
      at node_modules/resolve/lib/async.js:99:17
      at node_modules/resolve/lib/async.js:97:35
      at processDirs (node_modules/resolve/lib/async.js:244:39)
      at isdir (node_modules/resolve/lib/async.js:251:32)
      at node_modules/resolve/lib/async.js:23:69
      at FSReqWrap.oncomplete (fs.js:154:21)

  4) deep module reference mapping without file extension - .json:
     Uncaught AssertionError [ERR_ASSERTION]: ifError got unwanted exception: Cannot find module 'module-n/bar' from '/home/vagrant/forge/debian/git/js-team/node-browser-resolve/test/fixtures'
      at test/modules.js:113:16
      at index.js:265:24
      at node_modules/resolve/lib/async.js:99:17
      at node_modules/resolve/lib/async.js:97:35
      at processDirs (node_modules/resolve/lib/async.js:244:39)
      at isdir (node_modules/resolve/lib/async.js:251:32)
      at node_modules/resolve/lib/async.js:23:69
      at FSReqWrap.oncomplete (fs.js:154:21)

  5) alt-browser deep module reference mapping:

      Uncaught AssertionError [ERR_ASSERTION]: '/home/vagrant/forge/debian/git/js-team/node-browser-resolve/test/fixtures/node_modules/alt-browser-field/direct.js' == '/home/vagrant/forge/debian/git/js-team/node-browser-resolve/test/fixtures/node_modules/alt-browser-field/chromeapp-direct.js'
      + expected - actual

      -/home/vagrant/forge/debian/git/js-team/node-browser-resolve/test/fixtures/node_modules/alt-browser-field/direct.js
      +/home/vagrant/forge/debian/git/js-team/node-browser-resolve/test/fixtures/node_modules/alt-browser-field/chromeapp-direct.js

      at test/modules.js:292:16
      at index.js:269:13
      at node_modules/resolve/lib/async.js:93:25
      at maybeUnwrapSymlink (node_modules/resolve/lib/async.js:35:9)
      at node_modules/resolve/lib/async.js:89:24
      at onfile (node_modules/resolve/lib/async.js:258:27)
      at onex (node_modules/resolve/lib/async.js:161:32)
      at node_modules/resolve/lib/async.js:11:20
      at FSReqWrap.oncomplete (fs.js:155:5)

  6) not fail on accessing path name defined in Object.prototype:

      Uncaught AssertionError [ERR_ASSERTION]: 'toString' == '/home/vagrant/forge/debian/git/js-team/node-browser-resolve/test/fixtures/node_modules/toString/index.js'
      + expected - actual

      -toString
      +/home/vagrant/forge/debian/git/js-team/node-browser-resolve/test/fixtures/node_modules/toString/index.js

      at test/modules.js:317:16
      at index.js:269:13
      at node_modules/resolve/lib/async.js:87:38
      at ondir (node_modules/resolve/lib/async.js:264:27)
      at onex (node_modules/resolve/lib/async.js:161:32)
      at node_modules/resolve/lib/async.js:11:20
      at FSReqWrap.oncomplete (fs.js:155:5)
nicolo-ribaudo commented 4 years ago

I'm working on this.

guimard commented 4 years ago

Hi, even with last resolve (which includes the 2 fixes), test continues to fail

guimard commented 4 years ago

New failures are:

  1) index.js of module dir:
     Uncaught AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
+ expected - actual                                                                                                                                                                 

- {                                                                                                                                                                                 
-   main: 'fixtures'                                                                                                                                                                
- }                                                                                                                                                                                 
+ undefined                                                                                                                                                                         
      at test/modules.js:11:16                                                                                                                                                      
      at index.js:269:13                                                                                                                                                            
      at /usr/share/nodejs/resolve/lib/async.js:93:25                                                                                                                               
      at maybeUnwrapSymlink (/usr/share/nodejs/resolve/lib/async.js:35:9)                                                                                                           
      at /usr/share/nodejs/resolve/lib/async.js:89:24                                                                                                                               
      at ondir (/usr/share/nodejs/resolve/lib/async.js:270:27)                                                                                                                      
      at onex (/usr/share/nodejs/resolve/lib/async.js:161:32)                                                                                                                       
      at /usr/share/nodejs/resolve/lib/async.js:11:20                                                                                                                               
      at FSReqWrap.oncomplete (fs.js:154:5)                                                                                                                                         

  2) deep module reference mapping:
     Uncaught AssertionError [ERR_ASSERTION]: ifError got unwanted exception: Cannot find module 'module-l/direct' from '/<<PKGBUILDDIR>>/test/fixtures'
      at test/modules.js:95:16                                                                                                                                                      
      at index.js:265:24                                                                                                                                                            
      at /usr/share/nodejs/resolve/lib/async.js:99:17                                                                                                                               
      at /usr/share/nodejs/resolve/lib/async.js:97:35                                                                                                                               
      at processDirs (/usr/share/nodejs/resolve/lib/async.js:250:39)                                                                                                                
      at ondir (/usr/share/nodejs/resolve/lib/async.js:271:13)                                                                                                                      
      at load (/usr/share/nodejs/resolve/lib/async.js:137:43)                                                                                                                       
      at onex (/usr/share/nodejs/resolve/lib/async.js:162:17)                                                                                                                       
      at /usr/share/nodejs/resolve/lib/async.js:13:69                                                                                                                               
      at FSReqWrap.oncomplete (fs.js:153:21)                                                                                                                                        

  3) deep module reference mapping without file extension - .js:
     Uncaught AssertionError [ERR_ASSERTION]: ifError got unwanted exception: Cannot find module 'module-n/foo' from '/<<PKGBUILDDIR>>/test/fixtures'
      at test/modules.js:106:16                                                                                                                                                     
      at index.js:265:24                                                                                                                                                            
      at /usr/share/nodejs/resolve/lib/async.js:99:17                                                                                                                               
      at /usr/share/nodejs/resolve/lib/async.js:97:35                                                                                                                               
      at processDirs (/usr/share/nodejs/resolve/lib/async.js:250:39)                                                                                                                
      at ondir (/usr/share/nodejs/resolve/lib/async.js:271:13)                                                                                                                      
      at load (/usr/share/nodejs/resolve/lib/async.js:137:43)                                                                                                                       
      at onex (/usr/share/nodejs/resolve/lib/async.js:162:17)                                                                                                                       
      at /usr/share/nodejs/resolve/lib/async.js:13:69                                                                                                                               
      at FSReqWrap.oncomplete (fs.js:153:21)                                                                                                                                        

  4) deep module reference mapping without file extension - .json:
     Uncaught AssertionError [ERR_ASSERTION]: ifError got unwanted exception: Cannot find module 'module-n/bar' from '/<<PKGBUILDDIR>>/test/fixtures'
      at test/modules.js:113:16                                                                                                                                                     
      at index.js:265:24                                                                                                                                                            
      at /usr/share/nodejs/resolve/lib/async.js:99:17                                                                                                                               
      at /usr/share/nodejs/resolve/lib/async.js:97:35                                                                                                                               
      at processDirs (/usr/share/nodejs/resolve/lib/async.js:250:39)                                                                                                                
      at ondir (/usr/share/nodejs/resolve/lib/async.js:271:13)                                                                                                                      
      at load (/usr/share/nodejs/resolve/lib/async.js:137:43)                                                                                                                       
      at onex (/usr/share/nodejs/resolve/lib/async.js:162:17)                                                                                                                       
      at /usr/share/nodejs/resolve/lib/async.js:13:69                                                                                                                               
      at FSReqWrap.oncomplete (fs.js:153:21)                                                                                                                                        

  5) alt-browser deep module reference mapping:

      Uncaught AssertionError [ERR_ASSERTION]: '/<<PKGBUILDDIR>>/test/fixtures/node_modules/alt-browser-field/direct.js' == '/<<PKGBUILDDIR>>/test/fixtures/node_modules/alt-browser-field/chromeapp-direct...                                                                                                                                                          
      + expected - actual

      -/<<PKGBUILDDIR>>/test/fixtures/node_modules/alt-browser-field/direct.js
      +/<<PKGBUILDDIR>>/test/fixtures/node_modules/alt-browser-field/chromeapp-direct.js

      at test/modules.js:292:16                                                                                                                                                     
      at index.js:269:13                                                                                                                                                            
      at /usr/share/nodejs/resolve/lib/async.js:93:25                                                                                                                               
      at maybeUnwrapSymlink (/usr/share/nodejs/resolve/lib/async.js:35:9)                                                                                                           
      at /usr/share/nodejs/resolve/lib/async.js:89:24                                                                                                                               
      at onfile (/usr/share/nodejs/resolve/lib/async.js:264:27)                                                                                                                     
      at onex (/usr/share/nodejs/resolve/lib/async.js:161:32)                                                                                                                       
      at /usr/share/nodejs/resolve/lib/async.js:11:20                                                                                                                               
      at FSReqWrap.oncomplete (fs.js:154:5)                                                                                                                                         

  6) not fail on accessing path name defined in Object.prototype:
     Uncaught AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
+ expected - actual                                                                                                                                                                 

- {                                                                                                                                                                                 
-   main: 'fixtures'                                                                                                                                                                
- }                                                                                                                                                                                 
+ undefined                                                                                                                                                                         
      at test/modules.js:318:16                                                                                                                                                     
      at index.js:269:13                                                                                                                                                            
      at /usr/share/nodejs/resolve/lib/async.js:93:25                                                                                                                               
      at maybeUnwrapSymlink (/usr/share/nodejs/resolve/lib/async.js:35:9)                                                                                                           
      at /usr/share/nodejs/resolve/lib/async.js:89:24                                                                                                                               
      at ondir (/usr/share/nodejs/resolve/lib/async.js:270:27)                                                                                                                      
      at onex (/usr/share/nodejs/resolve/lib/async.js:161:32)                                                                                                                       
      at /usr/share/nodejs/resolve/lib/async.js:11:20                                                                                                                               
      at FSReqWrap.oncomplete (fs.js:154:5)
NilSet commented 4 years ago

A newer version of resolve is also needed to be compatible with yarn 2's Plug'n'Play default linking strategy.

https://next.yarnpkg.com/advanced/migration#make-sure-you-use-resolve19

ljharb commented 4 years ago

@guimard please let me know if there's still test failures on the latest version of resolve.

arcanis commented 4 years ago

There are seven fails at the moment:

  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․

  61 passing (84ms)
  7 failing

  1)  index.js of module dir:
     Uncaught AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ {
+   main: 'fixtures'
+ }
- undefined
      at /Users/mael.nison/node-browser-resolve/test/modules.js:11:16
      at /Users/mael.nison/node-browser-resolve/index.js:230:25
      at LOOP (fs.js:1576:14)
      at processTicksAndRejections (internal/process/task_queues.js:75:11)

  2)  deep module reference mapping:
     Uncaught TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be one of type string, Buffer, or URL. Received type undefined
      at Object.stat (fs.js:828:10)
      at cb (/Users/mael.nison/node-browser-resolve/index.js:220:12)
      at /Users/mael.nison/node-browser-resolve/index.js:283:24
      at /Users/mael.nison/node-browser-resolve/node_modules/resolve/lib/async.js:109:17
      at processDirs (/Users/mael.nison/node-browser-resolve/node_modules/resolve/lib/async.js:260:39)
      at isdir (/Users/mael.nison/node-browser-resolve/node_modules/resolve/lib/async.js:267:32)
      at /Users/mael.nison/node-browser-resolve/node_modules/resolve/lib/async.js:23:69
      at FSReqCallback.oncomplete (fs.js:165:21)

  3)  deep module reference mapping without file extension - .js:
     Uncaught TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be one of type string, Buffer, or URL. Received type undefined
      at Object.stat (fs.js:828:10)
      at cb (/Users/mael.nison/node-browser-resolve/index.js:220:12)
      at /Users/mael.nison/node-browser-resolve/index.js:283:24
      at /Users/mael.nison/node-browser-resolve/node_modules/resolve/lib/async.js:109:17
      at processDirs (/Users/mael.nison/node-browser-resolve/node_modules/resolve/lib/async.js:260:39)
      at isdir (/Users/mael.nison/node-browser-resolve/node_modules/resolve/lib/async.js:267:32)
      at /Users/mael.nison/node-browser-resolve/node_modules/resolve/lib/async.js:23:69
      at FSReqCallback.oncomplete (fs.js:165:21)

  4)  deep module reference mapping without file extension - .json:
     Uncaught TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be one of type string, Buffer, or URL. Received type undefined
      at Object.stat (fs.js:828:10)
      at cb (/Users/mael.nison/node-browser-resolve/index.js:220:12)
      at /Users/mael.nison/node-browser-resolve/index.js:283:24
      at /Users/mael.nison/node-browser-resolve/node_modules/resolve/lib/async.js:109:17
      at processDirs (/Users/mael.nison/node-browser-resolve/node_modules/resolve/lib/async.js:260:39)
      at isdir (/Users/mael.nison/node-browser-resolve/node_modules/resolve/lib/async.js:267:32)
      at /Users/mael.nison/node-browser-resolve/node_modules/resolve/lib/async.js:23:69
      at FSReqCallback.oncomplete (fs.js:165:21)

  5)  alt-browser deep module reference mapping:

      + expected - actual

      +/Users/mael.nison/node-browser-resolve/test/fixtures/node_modules/alt-browser-field/chromeapp-direct.js
      -/Users/mael.nison/node-browser-resolve/test/fixtures/node_modules/alt-browser-field/direct.js

      at /Users/mael.nison/node-browser-resolve/test/modules.js:292:16
      at /Users/mael.nison/node-browser-resolve/index.js:230:25
      at LOOP (fs.js:1576:14)
      at processTicksAndRejections (internal/process/task_queues.js:75:11)

  6)  not fail on accessing path name defined in Object.prototype:
     Uncaught AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ {
+   main: 'fixtures'
+ }
- undefined
      at /Users/mael.nison/node-browser-resolve/test/modules.js:318:16
      at /Users/mael.nison/node-browser-resolve/index.js:230:25
      at LOOP (fs.js:1576:14)
      at processTicksAndRejections (internal/process/task_queues.js:75:11)

  7)  respect symlinks:
     Uncaught AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ {
+   main: 'fixtures'
+ }
- undefined
      at /Users/mael.nison/node-browser-resolve/test/modules.js:340:16
      at /Users/mael.nison/node-browser-resolve/index.js:230:25
      at LOOP (fs.js:1576:14)
      at processTicksAndRejections (internal/process/task_queues.js:75:11)
arcanis commented 4 years ago

@ljharb I investigated a bit and the regression started in 1.7.0 because of https://github.com/browserify/resolve/pull/152

Reverting the changes causes the tests to pass (even in 1.15.1). I'm not sure what that entails though, or whether the problem should be fixed by resolve or browser-resolve 🙂

ljharb commented 4 years ago

Hmm - the implication is indeed that browser-resolve is relying on the bug that that PR fixed :-/

To be honest, though, some of those tests (1, 6, 7) just look like they're testing unimportant info - ie, it's like a snapshot test, where you don't actually care what the value is, but you've still asserted on it anyways. Perhaps those assertions can either be updated, or removed, or perhaps I'm missing something vital?

The other 4 tests look important, though - they're all passing in undefined to fs.stat where perhaps previously the package option (and thus, cb) wasn't being called at all? Given that all the test cases that needed updating in https://github.com/browserify/resolve/pull/152 involved receiving package data where undefined was previously called, that may also be relevant.

guimard commented 4 years ago

Hi,

This (revert) patch fixes browser-resolve tests without changing resolve tests:

--- a/lib/async.js
+++ b/lib/async.js
@@ -265,13 +265,13 @@
         function isdir(err, isdir) {
             if (err) return cb(err);
             if (!isdir) return processDirs(cb, dirs.slice(1));
-            loadAsFile(dir, opts.package, onfile);
+            loadAsFile(dir, undefined, onfile);
         }

         function onfile(err, m, pkg) {
             if (err) return cb(err);
             if (m) return cb(null, m, pkg);
-            loadAsDirectory(dir, opts.package, ondir);
+            loadAsDirectory(dir, undefined, ondir);
         }

         function ondir(err, n, pkg) {
ljharb commented 4 years ago

@guimard well sure, but that reverts a bugfix in resolve. The fix for browser-resolve is likely going to involve changing browser-resolve.

the-spyke commented 4 years ago

Any news on this? Yarn 2 PnP usage depends on this update.

goto-bus-stop commented 4 years ago

I think I updated tests correctly for browser-resolve in https://github.com/browserify/browser-resolve/compare/resolve-twelve, but there are a lot of test failures in browserify with that change that will need more investigation.

ljharb commented 4 years ago

@goto-bus-stop can you link me to the browserify test failures?

goto-bus-stop commented 4 years ago

The majority actually seem to be caused by https://github.com/browserify/browser-resolve/pull/92. If there are any remaining ones not related to that i'll push a branch to browserify and let you take a look :innocent:

goto-bus-stop commented 4 years ago

92 is made obsolete by resolve's preserveSymlinks option, so unfortunately that PR will have to be (partially) reverted. In #97 I removed the implementation changes but kept the test. The remaining browserify failures are

  1. an error message assertion, which is intended, and
  2. something in the browser_field_resolve test that I don't really understand yet: https://travis-ci.org/github/browserify/browserify/jobs/699849991#L245 I'm not sure if it's related to the resolve bugfix, but maybe something stands out to you in test/browser_field_resolve/k?

(it's getting late here so I'll call it quits for today!)

ljharb commented 4 years ago

hmm - nothing jumps out at me. if that's the only failure tho, then could you try bisecting a bit with resolve versions? preserveSymlinks has been in there since resolve v1.4 so theoretically that might help expose what might be a bug in resolve?

goto-bus-stop commented 4 years ago

Finally took some time to dive into this. The failure starts at resolve 1.7.0 with the opts.package bugfix. What's happening is that we have a folder structure like this, and we try to bundle file.js:

root
- package.json
- file.js
- node_modules/dep/package.json
- node_modules/dep/otherfile.js

Now when file.js does require('dep/otherfile'), browserify passes in the closest package.json to file.js in the opts.package parameter. resolve finds node_modules/dep/otherfile.js, and since loadAsFile already has a pkg value from opts.package, it doesn't read the node_modules/dep/package.json. So dep's package.json never gets passed to opts.packageFilter, which browser-resolve uses for the browser field shimming. And resolve calls the callback cb(err, res, pkg) with the root package.json, not dep's package.json.

@ljharb Is that the intended behaviour for opts.package? I had assumed that I would pass in the package.json closest the file that I'm resolving from, but that I would still receive the package.json closest to the file that I'm resolving to in the callback. It does work that way if you resolve to a directory (require('dep')). But at first glance it seems like the way to do that is to partially revert the bugfix from resolve@1.7.0, for the loadAsFile() call? :/ Maybe my understanding is wrong here—we can also work around this in browserify by just not passing in an opts.package value, which I think is just intended to reduce fs calls.

ljharb commented 4 years ago

@goto-bus-stop thanks for the research!

Is the bugfix you're referring to https://github.com/browserify/resolve/pull/152?

The original creation of opts.package doesn't make it particularly clear to me what the intention was (it predates my involvement). I'd say that for now, browserify should omit opts.package so it can get the resolve update landed - but in the meantime, maybe you could open up a PR with a failing test case?

goto-bus-stop commented 4 years ago

Right, yes, https://github.com/browserify/resolve/pull/152.

I'll publish this as a major version then so older browserify versions don't auto-upgrade.