Skip to content
This repository has been archived by the owner on Nov 15, 2018. It is now read-only.

ensure inline requires are searched local dependencies are ignored #45

Merged
merged 1 commit into from
Nov 1, 2016

Conversation

evanshortiss
Copy link
Contributor

Great module, and something I found the need for just the other day during a PR review! I've found a small bug and patched it. Here's an overview:

My project had a file with these lines:

asyncThings()
  .then(require('a-module'))

The above caused an issue in the searcher logic since it relied on a require being assigned a variable (checking for an "=" symbol). It now no longer expects a require to be assigned.

Another minor issue I found was the missing.includes('./') piece in searcher.js. When using NODE_PATH=. we can perform local requires like so require('lib/a/b.js') without the need for a leading ./. This PR should address this also.

I will try add some tests for the searchDependencies function that might catch these cases to prevent regression.

@coveralls
Copy link

coveralls commented Nov 1, 2016

Coverage Status

Coverage remained the same at 92.593% when pulling f6a5afb on evanshortiss:inline-require into fd2dfe9 on bucharest-gold:master.

@coveralls
Copy link

coveralls commented Nov 1, 2016

Coverage Status

Coverage remained the same at 92.593% when pulling 5b9d673 on evanshortiss:inline-require into fd2dfe9 on bucharest-gold:master.

@helio-frota
Copy link
Member

Hi, thanks for the PR!
This improvement is interesting and necessary. 👍

But I can't get this working, I'm still getting a dependency as unused:

----------------------------------------------------------------------
[ Unused dependencies ]
----------------------------------------------------------------------
roi

I used this [Foo] project to do the test:

https://github.com/panther-js/lbe

The roi dependency: https://github.com/panther-js/lbe/blob/master/package.json#L17

The usage: https://github.com/panther-js/lbe/blob/master/server.js#L3-L12

The szero pointing to your PR branch locally: https://github.com/panther-js/lbe/blob/master/package.json#L26

for me to be sure I'm using your PR branch:

hf@archT440 ~/p/lbe ±master » cat node_modules/szero/lib/searcher.js | grep "Plucks"                                                                                                                           1 ↵
 * Plucks a require statment from a given line of code, e.g passing the line
 * Plucks the specific require string from a require statment, e.g given

@evanshortiss
Copy link
Contributor Author

@helio-frota will take a look sometime today 👍

@evanshortiss
Copy link
Contributor Author

@helio-frota, just had a quick look and found that the search for unused was also checking for "=", updated with the new logic. Thanks for testing!

@coveralls
Copy link

coveralls commented Nov 1, 2016

Coverage Status

Coverage decreased (-2.5%) to 90.076% when pulling ce0380f on evanshortiss:inline-require into fd2dfe9 on bucharest-gold:master.

@helio-frota
Copy link
Member

@evanshortiss thanks!
This is working.

But something happened with amount of usage , look:

hf@archT440 ~/p/lbe ±master » npm run dependencyCheck

> lbe@0.0.1 dependencyCheck /mnt/datadisk/lbe
> szero . --ci

----------------------------------------------------------------------
[ Declaration and file lines ]
----------------------------------------------------------------------
hapi-require('hapi'):
./server.js:22
lightbright-require('lightbright'):
./server.js:16
./server.js:17
./server.js:18
./server.js:19
./server.js:60
ip-require('ip'):
./server.js:15
./server.js:27
roi-require('roi'):
./server.js:7   <-
./server.js:9  <-----
./server.js:12 <====== 
./server.js:7  <-
./server.js:9  <-----
./server.js:12 <======
./server.js:7  <-
./server.js:9  <-----
./server.js:12 <======
----------------------------------------------------------------------
[ Declaration and amount ]
----------------------------------------------------------------------
hapi-require('hapi') --> [ 1 ]
lightbright-require('lightbright') --> [ 5 ]
ip-require('ip') --> [ 2 ]
roi-require('roi') --> [ 9 ]   <-----------------------------------
----------------------------------------------------------------------
[ Unused dependencies ]
----------------------------------------------------------------------
None.
----------------------------------------------------------------------
[ Licenses ]
----------------------------------------------------------------------
roi --> Apache-2.0
hapi --> BSD-3-Clause
lightbright --> Apache-2.0
ip --> MIT

I'm not sure if this change discovered a hidden bug or added a new one.

@coveralls
Copy link

coveralls commented Nov 1, 2016

Coverage Status

Coverage decreased (-2.5%) to 90.076% when pulling d342a1d on evanshortiss:inline-require into fd2dfe9 on bucharest-gold:master.

@helio-frota
Copy link
Member

@evanshortiss also for unit tests you can add a dependency on this file:
https://github.com/bucharest-gold/szero/blob/master/test/fixtures/package.json

Then you can add the usage myFunction().then(require('theDependencyAdded^'));
inside any file you want on the directories foo or xpto:
https://github.com/bucharest-gold/szero/tree/master/test/fixtures

@coveralls
Copy link

coveralls commented Nov 1, 2016

Coverage Status

Coverage increased (+0.9%) to 93.511% when pulling cb1a7e8 on evanshortiss:inline-require into fd2dfe9 on bucharest-gold:master.

@evanshortiss
Copy link
Contributor Author

@helio-frota sure thing, will update shortly.

@coveralls
Copy link

coveralls commented Nov 1, 2016

Coverage Status

Coverage increased (+3.07%) to 93.511% when pulling 3551578 on evanshortiss:inline-require into a80b028 on bucharest-gold:master.

@helio-frota helio-frota added this to the 0.7.0 milestone Nov 1, 2016
@helio-frota helio-frota merged commit 1725920 into bucharest-gold:master Nov 1, 2016
@helio-frota
Copy link
Member

Thanks!
Also, I'm going to open a new Issue related with report amount duplication, because I have no idea if this feature shows a hidden bug or add a new one.

@evanshortiss
Copy link
Contributor Author

Awesome, thanks! Yeah, I think I saw what you mentioned, but think it always existed?

@evanshortiss
Copy link
Contributor Author

Oh wait. Not sure on this. Can try take a look.

@helio-frota
Copy link
Member

@evanshortiss this was a hidden existing bug.
because in common cases nobody will do something like:

const a = require('http');
const b = require('http');
const c = require('http');

-- > In the same file. ^ < ---

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

3 participants