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

System js support #29

Closed
ghost opened this issue Mar 2, 2016 · 16 comments
Closed

System js support #29

ghost opened this issue Mar 2, 2016 · 16 comments

Comments

@ghost
Copy link

ghost commented Mar 2, 2016

@mrjoelkemp: Hi, I like your dependency-tree tool. But unfortunately it doesn't meet my requirements. I use SystemsJS that uses a system.config.js file to map the paths of modules to specific name. See below:

import * from 'angular'
import lodash from 'lodash'

with 'lodash' declared in the system.config.js like so:

'lodash' : 'node_modules/lodash/lodash.js'
'angular' : 'lib/angular/angular.js'

So essentially module path resolution should take place based on the keys in your system.config.js I've looked at the code and I would love to try my hand at adding support for SystemJS. I've already played with your code and I have the basics working.

@mrjoelkemp
Copy link
Collaborator

Hi Marthinus,

Thanks for contributing!

Webpack and SystemJS support are definitely items on the wish list. I'm
definitely open to PRs adding support for either.

Before a PR, I'd like to understand the changes that are necessary to
provide SystemJS support. Feel free to provide some notes in a comment on
this thread, or point me to your fork so that we can be on the same page.

It's definitely possible to add support in dependency-tree core, but my
hunch is that the resolution logic should happen at the filing-cabinet
level since that's the resolution factory. Happy to discuss further.
On Mar 2, 2016 5:56 PM, "Marthinus Engelbrecht" notifications@github.com
wrote:

@mrjoelkemp https://github.com/mrjoelkemp: Hi, I like your
dependency-tree tool. But unfortunately it doesn't meet my requirement. I
use SystemsJS https://github.com/systemjs/systemjs that uses a
system.config.js file to map the paths of modules to specific name. See
below:

import * from 'angular'import lodash from 'lodash'

with 'lodash' declared in the system.config.js like so:

'lodash' : 'node_modules/lodash/lodash.js''angular' : 'lib/angular/angular.js'

So essentially module path resolution should take place based on the keys
in your system.config.js I've looked at the code and I would love to try
my hand at adding support for SystemJS. I've already played with your code
and I have the basics working.


Reply to this email directly or view it on GitHub
#29.

@ghost
Copy link
Author

ghost commented Mar 3, 2016

@mrjoelkemp: Yeah after looking at the code more I also came to the conclusion that it should be in filling cabinet, somewhere around here. I believe it would simply be a matter of adding a lookup function specific to 'systemjs' that would just use it's config which is just an associative array to look up the full paths. Distinguishing between 'systemjs' and 'es6' modules would be a bit more tricky, but we can maybe just check if there are any forward slashes('/') in the path. I haven't forked anything yet, just did a clone. But If you cool with it I will fork filling-cabinet try the above suggestion and once I have something that works I will point you to it.

@ghost
Copy link
Author

ghost commented Mar 3, 2016

We might have to refactor resolve-dependency-path, to remove the SystemJS plugin handling here and place it in the systemLookup function

@mrjoelkemp
Copy link
Collaborator

@marthinus-engelbrecht Thanks for looking into it.

I think we could introduce an artificial type systemjs here https://github.com/mrjoelkemp/node-filing-cabinet/blob/master/index.js#L78-L81. To not muddy a requirejs config with a systemjs or webpack config, we can introduce new params like systemjsConfig and webpackConfig. So if the module is es6 and we have a systemjsConfig, then we can say that the type is systemjs and use a custom systemjs resolver.

@mrjoelkemp
Copy link
Collaborator

We can also have dependency-tree support the new systemjsConfig option as a passthrough to filing-cabinet around here: https://github.com/mrjoelkemp/node-dependency-tree/blob/master/index.js#L190.

Happy to look at some code and help answer any questions you might have. Super excited for this!

@ghost
Copy link
Author

ghost commented Mar 3, 2016

Yeah that's what i've also been thinking. I'll see if I can get a chance to work on this tomorrow, otherwise Saturday.

@ghost
Copy link
Author

ghost commented Mar 3, 2016

As a side note. What made you decide to create this library instead of contributing to something like madge? I'm considering seeing if I can add support there as well.

@mrjoelkemp
Copy link
Collaborator

Great question. There's actually an open issue to integrate dependency-tree into madge. Madge is a visualization tool that needs to trace the dependency tree as a pre-req; it's not a dependency tree generation tool.

I built this and many of the other libraries for https://github.com/mrjoelkemp/dependents without really knowing that Madge was tackling a subset of my needs. Ideally, I would have extracted the tree-generation stuff from Madge, but I took a bottom-up approach of building core libraries used for other parts of Dependents that were also leveraged in dependency-tree.

@ghost
Copy link
Author

ghost commented Mar 4, 2016

Cool. Makes sense to me. I like bottom up. I was looking at madge, and I was like "ok this is pretty cool, but I don't really need all the visualisation stuff". It would be really cool to see node-dependency-tree be used by madge.

@ghost
Copy link
Author

ghost commented Mar 4, 2016

We have a slight problem. The System.js config file isn't json, and you actually have to call System.config() inside it to set it up. So to get hold of the config object inside filing-cabinet it would have to know about SystemJS, at least if we want it to be passed in as a command line argument. I created an issue with SystemJS, systemjs/systemjs#1140, let's see what they say.

@mrjoelkemp
Copy link
Collaborator

Thanks for looking into it. Check out
https://www.npmjs.com/package/requirejs-config-file for ideas on a
scrappier way of parsing the config without systemjs. The linked module is
for requirejs, but the technique is applicable.
On Mar 4, 2016 4:31 PM, "Marthinus Engelbrecht" notifications@github.com
wrote:

We have a slight problem. The System.js config file isn't json, and you
actually have to call System.config() inside it to set it up. So to get
hold of the config object inside filing-cabinet it would have to know about
SystemJS, at least if we want it to be passed in as a command line
argument. I created an issue with SystemJS, systemjs/systemjs#1140
systemjs/systemjs#1140, let's see what they
say.


Reply to this email directly or view it on GitHub
#29 (comment)
.

@ghost
Copy link
Author

ghost commented Mar 22, 2016

Ok, closed the system js issue. The system-builder gives us all that we need. I've forked filling-cabinet, will be adding some code soon.

@mrjoelkemp
Copy link
Collaborator

Sounds awesome! Thanks for sticking with this!
On Mar 22, 2016 4:37 AM, "Marthinus Engelbrecht" notifications@github.com
wrote:

Ok, closed the system js issue. The system-builder
https://github.com/systemjs/builder gives us all that we need. I've
forked filling-cabinet
https://github.com/marthinus-engelbrecht/node-filing-cabinet, will be
adding some code soon.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#29 (comment)

@mrjoelkemp
Copy link
Collaborator

@marthinus-engelbrecht Hey! Any progress on the SystemJS front? I just landed webpack support in node-dependency-tree and was thinking about tackling SystemJS. Any help is appreciated.

@ghost
Copy link
Author

ghost commented May 12, 2016

@mrjoelkemp Yes there has been. Sorry, I've been a bit swamped with work. You can take a look at it here. Will see if I can get some work done this weekend. I played around a bit as well with generating json-graph and then transpiling it to a dot format and creating a graph with graphviz

@mrjoelkemp
Copy link
Collaborator

@marthinus-engelbrecht Awesome! It looks really good. I'd definitely use it within filing-cabinet (once BinaryCraft/systemjs-absolute-path-resolver#1 has been addressed). Thanks for the update!

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

No branches or pull requests

1 participant