Replaced read-pkg-up with cache-aware package locator#1017
Replaced read-pkg-up with cache-aware package locator#1017hulkish wants to merge 5 commits intoimport-js:mainfrom
Conversation
hulkish
force-pushed
the
cached-read-pkg-up
branch
from
a9fb1b0 to
09fa103
Compare
| // if not found, allopw search to continue | ||
| if (err.code !== 'ENOENT') throw err | ||
| } | ||
| } while (dirname !== (dirname = path.dirname(dirname))) |
There was a problem hiding this comment.
is there a way this can be written without conflating an assignment with an expression?
There was a problem hiding this comment.
@ljharb Yes, but this plays nicer with the goal of "fail fast". Meaning, if i were to replace with a typical condition-first loop such as for/while - it would require me to add logic which ensured the initial dirname was searched.
|
|
||
| const response = super.readPackageSync(location) | ||
| this.store[location] = response.result | ||
| return response |
There was a problem hiding this comment.
if we only ever use the result, why return the entire response?
There was a problem hiding this comment.
@ljharb It's needed so that readPackageUpSync can report all locations which failed to yield a file - in the event that no fles were located.
1 similar comment
hulkish
force-pushed
the
cached-read-pkg-up
branch
from
04eab1d to
efe80dd
Compare
|
@ljharp one other thing that occurs to me... is it safe for the cache to exist at module level as i currently have it? Are there instances where I should be clearing the cache? I'd like to add some tests to account for this. |
|
@ljharb Ok I think this is ready now. One easy way to see the benefit here, is by running the following command in both BABEL_ENV=test NODE_PATH=./src ./node_modules/.bin/mocha --require babel-register tests/src/rules/no-extraneous-dependencies.js --trace-sync-io 2>&1 | grep -c "src/rules/no-extraneous-dependencies.js"The difference is clear when u see that master produces 524 io calls originating from the |
hulkish
force-pushed
the
cached-read-pkg-up
branch
5 times, most recently
from
0a298c4 to
3c0aeb3
Compare
| import os from 'os' | ||
| import fs from 'fs' | ||
|
|
||
| export default class CachedPackageLocator { |
There was a problem hiding this comment.
i wonder if this would be better extracted to its own published package with its own tests - is that something you'd be willing to publish and maintain?
There was a problem hiding this comment.
Hmm sure, I'll try to find some some for this either today or this week.
| const options = context.options[0] || {} | ||
| const filename = context.getFilename() | ||
| const deps = getDependencies(context, options.packageDir) | ||
| const deps = packageLocator.readUpSync( |
There was a problem hiding this comment.
it seems kind of odd to instantiate packageLocator only once and then use it only once to get at the readUpSync instance method. Perhaps readUpSync could just be exported directly as a singleton function, or perhaps CachedPackageLocator could be a factory function instead?
|
|
||
| before(function () { | ||
| sandbox = sinon.sandbox.create() | ||
| sandbox.stub(fs, 'readFileSync').reset() |
There was a problem hiding this comment.
why would it need to be reset immediately after creating the stub?
There was a problem hiding this comment.
It won't work without it. Some kind of sinon bug, I gathered.
There was a problem hiding this comment.
What happens when it doesn't work?
…sed by slight change for error reporting
hulkish
force-pushed
the
cached-read-pkg-up
branch
2 times, most recently
from
154b3a7 to
14fdf06
Compare
|
|
||
| before(function () { | ||
| sandbox = sinon.sandbox.create() | ||
| sandbox.stub(fs, 'readFileSync').reset() |
There was a problem hiding this comment.
What happens when it doesn't work?
| }) | ||
|
|
||
| after(function () { | ||
| sandbox.restore() |
There was a problem hiding this comment.
I understand, that's what I had originally also - it does not initialize the stubbed method without calling reset, though. Try it yourself!
There was a problem hiding this comment.
I'll play with it; but in the meantime let's change all this stuff so the sandbox is created before but everything else is in a beforeEach/afterEach.
|
@hulkish are you still interested in completing this? |
3 participants
No description provided.