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

yarn.lock file #15

Open
jonaskello opened this issue Dec 4, 2016 · 8 comments
Open

yarn.lock file #15

jonaskello opened this issue Dec 4, 2016 · 8 comments

Comments

@jonaskello
Copy link

Just wanted to let you know I'm doing some experimenting over here with using the yarn.lock file as we originally discussed. Not sure if it will be useful but at least I am learning a lot :-). I would really like to make the system config a pure function of the lockfile. But I discovered that is not possible as we need to add package config from from the jspm-node conversion stuff. This part is also quite slow, would be nice to find a way to make it faster. Anyway, the system config is now a pure function of the lockfile and the extra jspm-config.

I'm basically doing the same thing as in this repo, just divided the steps a little different to get info from the lockfile rather than the node_modules folder. I was thinking we could add some code that looks for a yarn.lock file and uses it if present, otherwise falls back to scanning through node_modules. Maybe I could contribute something like this but we would have to re-arrange a bit to make that work. And as I said, not sure if it will be useful as the slow part is probably not scanning node_modules.

One thing I noticed is that I did not have to hard-code the node-libs packages using the short name in the map. It worked with the long names. What I mean is that this worked:

SystemJS.config({
  "paths": {
    "nm:": "node_modules/"
  },
  "map": {
    "jspm-nodelibs-assert": "nm:jspm-nodelibs-assert",
    "jspm-nodelibs-buffer": "nm:jspm-nodelibs-buffer",
    "jspm-nodelibs-child_process": "nm:jspm-nodelibs-child_process",
    "jspm-nodelibs-cluster": "nm:jspm-nodelibs-cluster",
 ...

If I understood correctly the code in this repo will map with the short names like "assert", "buffer" and so on.

@alexisvincent
Copy link
Owner

@jonaskello Great work :) Read my thoughts on using the lock file here. Basically the slow part is generating the augmented configs, not tracing node_modules (+- 50ms). Not sure that this can really be sped up much. But it's worth investigating and contributing back to jspm/npm.

I'm really interested about your claim that we don't need to map the short names (this will solve a bunch of hacks I think). One thing that makes me skeptical is that jspm does this. The only way this would be the case is if SystemJS does the mapping automatically. But I'll investigate.

Great work on the package.

One thing I couldn't workout from the yarn lock (I'm sure it's relatively easy to discover), is how yarn maps private node_modules.

@jonaskello
Copy link
Author

I also tried deleting the un-prefixed node-libs in the generated config in the example in this repo. It also works. But I don't understand why :-).

I'm also not sure how private node_modules are handled in yarn so i posted this issue.

@alexisvincent
Copy link
Owner

What's probably happening is that none of your packages actually depend on the core libs

@jonaskello
Copy link
Author

If I remove systemjs-nodelibsfrom package.jsonand re-install it will try to load /process so I guess it is dependent on that somehow.

@jonaskello
Copy link
Author

Btw, I'm using the react example from this repo so it loads react and react-dom.

@jjrv
Copy link
Collaborator

jjrv commented Dec 5, 2016

React just needs process.env.

@jonaskello
Copy link
Author

I see what's going on now. process is not among all the other node-libs at the end of the map, it is among the other dependencies so it did not get deleted when I deleted the last section of the map.

@jonaskello
Copy link
Author

And it seems that is because the process that react uses is not from the jspm-nodelibs at all. Instead it installs this package.

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

3 participants