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

Failing test for hashForDep(somePathDeepInPackage). #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rwjblue
Copy link
Collaborator

@rwjblue rwjblue commented Aug 13, 2016

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...

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
Copy link
Collaborator Author

rwjblue commented Aug 13, 2016

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

@rwjblue
Copy link
Collaborator Author

rwjblue commented Aug 13, 2016

See emberjs/emberjs-build#175 for where this cropped up.

@stefanpenner
Copy link
Owner

stefanpenner commented Aug 13, 2016

@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
Copy link
Owner

@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
Copy link
Owner

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...

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 this pull request may close these issues.

2 participants