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

vice versa function #41

Closed
lijunle opened this issue Nov 11, 2015 · 12 comments
Closed

vice versa function #41

lijunle opened this issue Nov 11, 2015 · 12 comments
Labels

Comments

@lijunle
Copy link
Member

lijunle commented Nov 11, 2015

From @js08 on October 28, 2015 18:20

Is it possble to identify the package name in package.json file when that module is not present. I mean can we do a vice versa function in depcheck-es6

Copied from original issue: lijunle#99

@lijunle
Copy link
Member Author

lijunle commented Nov 11, 2015

That is possible. Seems like a feature.

@js08 Are you going to implement it?

@lijunle
Copy link
Member Author

lijunle commented Nov 11, 2015

From @js08 on October 28, 2015 18:28

@lijunle it would be great if you provide us guidance, where to start with it

@lijunle
Copy link
Member Author

lijunle commented Nov 11, 2015

You need to read and understand the source code. I do roughly look on the code, the main modification could be here.

I suppose you have the knowledge about the JavaScript programming, ES6 syntax and Promise based asynchronous pattern. If not, please google around to learn them. They are very useful.

@lijunle
Copy link
Member Author

lijunle commented Nov 11, 2015

From @js08 on October 28, 2015 19:7

@lijunle I looked into the code, it would be great if you add some comments before each function,
If its a small fix, can you update at your end

@lijunle
Copy link
Member Author

lijunle commented Nov 11, 2015

  1. Do not expect great document, read the first sentence in my previous post.
  2. If you can not help, I will not start on it immediately - until I get some time.

@lijunle
Copy link
Member Author

lijunle commented Nov 11, 2015

From @js08 on October 28, 2015 20:6

@lijunle yeah I am looking at the code and debugging, is it a small fix

@lijunle
Copy link
Member Author

lijunle commented Nov 11, 2015

From @trkailash on October 28, 2015 20:41

@lijunle I shall help. What is the process to contribute ?

  1. fork and raise a PR ?
  2. Question with respect to the marked code above: I see "used" is an array of necessary dependencies. But, how do I recognize whether to add it to dependencies or devDependencies for the feature in question ?? Hint/pointers would give me a lead.
    Thanks !

@lijunle
Copy link
Member Author

lijunle commented Nov 11, 2015

Hi, @trkailash

  1. Yes, please fork and send a PR. I am welcome with that.
  2. No, you cann't. One more property beside to dependencies looks like a solution. Suppose name as notSaved or similar.
  3. I think a bit more on this, that is not so easy. The current design allows undetermined dependencies returned from detector. If we need to support this feature, the design need to change.

@lijunle
Copy link
Member Author

lijunle commented Nov 11, 2015

Besides, when providing such function, we will lost the chance to optimize the performance, because it already need to look all files to determine which package is not saved in package.json file.

However, I think, function is important than performance.

@lijunle
Copy link
Member Author

lijunle commented Nov 11, 2015

@trkailash I made a design change in #100. Please read the design and you will find the clue to implement this.

Thanks!
Junle

@lijunle
Copy link
Member Author

lijunle commented Nov 11, 2015

From @trkailash on November 1, 2015 14:53

Hello @lijunle I am a lot more familiar with the codebase now. I had a few questions to ask (some are elementary):

  1. How do I generate a new bin/depcheck ? I would like to generate this when I make a change to the source to run depcheck.
  2. From a solution standpoint I was thinking of running a third field called inclusions in the resolved promise (checkFile method). "inclusions" will be assigned to "used" for each file. We can collect all inclusions in a union (allInclusions) when all promises resolve and finally run minus(allInclusions, dep.concat(devDeps)) to get modules that are not in package.son.

Please share your thoughts on what you think about this approach ? Thanks !

@lijunle
Copy link
Member Author

lijunle commented Nov 11, 2015

Hi, @trkailash Thanks for your investigation.

How do I generate a new bin/depcheck ?

Run npm run prepublish, which is the standard hook before publish. However, you could just run npm run compile, because I suppose you will not add new components.

I would like to generate this when I make a change to the source to run depcheck.

However, a better way is to add unit test first, then you can verify your change without wait for bin/depcheck is generated.

From a solution standpoint I was thinking of running a third field called inclusions in the resolved promise (checkFile method). "inclusions" will be assigned to "used" for each file. We can collect all inclusions in a union (allInclusions) when all promises resolve and finally run minus(allInclusions, dep.concat(devDeps)) to get modules that are not in package.son.

That sounds good.

However, I could like to name both of inclusions and allInclusions to used, and the minus result as missing (notSaved is confusing).

After you do mius(used, dep.concat(devDeps)), please do not expose used. That keeps flexibility if we want to change something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant