stefanpenner / hash-for-dep

7 stars 11 forks source link

Failing test for `hashForDep(somePathDeepInPackage)`. #30

Open rwjblue opened 8 years ago

rwjblue commented 8 years ago

When calling:

hashForDep('/some/fqdn/package-name/lib/utils/');

An error with the following stack is triggered:

Error: Cannot find module '/Users/rjck/Source/tildeio/glimmer/node_modules/emberjs-build/lib/utils//package.json'
    at Function.Module._resolveFilename (module.js:440:15)
    at Function.Module._load (module.js:388:25)
    at Module.require (module.js:468:17)
    at require (internal/module.js:20:19)
    at pkg (/Users/rjck/Source/tildeio/glimmer/node_modules/hash-for-dep/lib/pkg.js:19:20)
    at statPathsFor (/Users/rjck/Source/tildeio/glimmer/node_modules/hash-for-dep/lib/stat-paths-for.js:13:20)
    at hashForDep (/Users/rjck/Source/tildeio/glimmer/node_modules/hash-for-dep/index.js:15:21)
    at Babel.optionsHash (/Users/rjck/Source/tildeio/glimmer/node_modules/broccoli-babel-transpiler/index.js:141:27)
    at Babel.cacheKeyProcessString (/Users/rjck/Source/tildeio/glimmer/node_modules/broccoli-babel-transpiler/index.js:186:15)
    at Object.module.exports.processString (/Users/rjck/Source/tildeio/glimmer/node_modules/broccoli-persistent-filter/lib/strategies/persistent.js:26:19)
    at Processor.processString (/Users/rjck/Source/tildeio/glimmer/node_modules/broccoli-persistent-filter/lib/processor.js:20:25)
    at /Users/rjck/Source/tildeio/glimmer/node_modules/broccoli-persistent-filter/index.js:231:16
    at lib$rsvp$$internal$$initializePromise (/Users/rjck/Source/tildeio/glimmer/node_modules/rsvp/dist/rsvp.js:1084:9)
    at new lib$rsvp$promise$$Promise (/Users/rjck/Source/tildeio/glimmer/node_modules/rsvp/dist/rsvp.js:546:53)
    at invoke (/Users/rjck/Source/tildeio/glimmer/node_modules/broccoli-persistent-filter/index.js:230:10)
    at Babel.Filter.processFile (/Users/rjck/Source/tildeio/glimmer/node_modules/broccoli-persistent-filter/index.js:248:16)

This is because module-base-dir does not attempt to resolve the package.json above the provided path.

Perhaps adding:

resolve.sync('package.json', { base: pathProvided})

Would fix, but I am uncertain...

rwjblue commented 8 years ago

@stefanpenner - I'm happy to fix this given some guidance...

rwjblue commented 8 years ago

See https://github.com/emberjs/emberjs-build/pull/175 for where this cropped up.

stefanpenner commented 8 years ago

@rwjblue: ok, well the issue is as follows:

module-base-dir today merely (and likely insufficiently) uses heuristics in the path to work. It looks at your path, and a/b/node_modules/c/b and assumes you have path-to-one-past-node-modules. Which most likely is too naive, and is actually not aligned with the node resolution algorithim.

This means, I believe we must actually inspect the disk, looking for package.json's to calculate the correct path.

So given:

/a/b/c/node_modules/d/e/f where f has a package.json, then the path is /a/b/c/node_modules/d/e/f but today we only inspect the string, which suggests /a/b/c/node_modules/d

/a/b/c/node_modules/d/e/f where e has the first “find-up” package.json, then the path is /a/b/c/node_modules/d/e /a/b/c/node_modules/d/e/f where d has the first “find-up” package.json, then the path is /a/b/c/node_modules/d /a/b/c/node_modules/d/e/f where c has the first “find-up” package.json, then the path is /a/b/c

I suspect the also change is like:

  1. Given /a/b/c/node_modules/d/e/f
  2. Walk from backwards from f, looking for package.json, when the first one is found, the path we are at should be the result of the current moduleBaseDir call
stefanpenner commented 8 years ago

@rwjblue actually looking at this again, it seems to be working as expected, or at the very least originally how I thought it should work. Now I am not sure which behavior we want. It does seem like it might be nice if it walked up until it found the package.json, although originally the idea was for subclasses to just provide it explicitly via baseDir.

It does seem like what you propose betters aligns us with the node ecosystem.

stefanpenner commented 5 years ago

this may actually be fixed now, if you have cycles it may be worth attempting to re-test against master.

A quick rebase proved tricky...