duojs / duo

A next-generation package manager for the front-end
3.42k stars 117 forks source link

Support "local" directive in component.json. #372

Closed beverlycodes closed 9 years ago

beverlycodes commented 10 years ago

Duo currently ignores the "local" and "paths" entries in component.json when resolving dependencies. This change resolves that by adding an isLocal test when resolving. It appends the dependency to all of the paths listed in the "paths" entry (and also tries root), returning the first one for which fs.exists reports true.

The isLocal test only gets used when component.json includes a "locals" entry. Otherwise, dependency resolution remains unchanged.

beverlycodes commented 10 years ago

I'm not sure why the Travis build failed. I ran the tests on my own machine against node 0.10.29 and 0.11.13 and everything passed.

stephenmathieson commented 10 years ago

why do you need local and paths? unlike component, you just require('./some/path') and you should be set.

edit: have you seen this: https://github.com/poying/duo-locals

beverlycodes commented 10 years ago

Two reasons come to mind.

Relative paths are fine for small apps, especially as a means for a quick start. They become a hindrance as an application grows in size and complexity. Duo supports a way to simplify requires for remote dependencies, but does not afford the same consideration for local dependencies. Component.json addressed this with local and paths.

As for duo-locals, I'm not keen on adopting a solution that works by monkey-patching Duo and is therefore subject to breaking anytime Duo (which is still evolving) changes some aspect of dependency resolution.

beverlycodes commented 10 years ago

I bumped my node installs up to 0.10.33 and 0.11.14. Tests still pass for me, so I'm wondering if the Travis failure was due to some issue in the Travis environment.

beverlycodes commented 10 years ago

Ah, I think I see a problem. I need to fix the exists call. co-fs exists is weird...

beverlycodes commented 10 years ago

Eh. Travis failed again, and as before I can't reproduce that failure locally. Node 0.10.33 and 0.11.14 both return a clean, all-pass run.

timaschew commented 10 years ago

Relative paths are fine for small apps, especially as a means for a quick start. They become a hindrance as an application grows in size and complexity. Duo supports a way to simplify requires for remote dependencies, but does not afford the same consideration for local dependencies. Component.json addressed this with local and paths.

:+1: :+1: :+1: Thanks that was exactly that what I tried to explain in some discussions component#250, duo#250

timaschew commented 10 years ago

there is something strange in the neighborhood..., seems to be too much network traffic, first time I got an error with node 0.10.24. but 0.11.13 passed fine. Travis has a timeout of 5000ms, my 2 timeout errors:

  1) Duo API .run([fn]) should build require conflicts:
     Error: timeout of 10000ms exceeded

  2) Duo API .run([fn]) with css should work with user/repo@ref:path:
     Error: timeout of 15000ms exceeded

With node 0.10.33 I got a totally different error, but again only for the first time.

  133 passing (2m)
  1 failing

  1) Duo Examples should build examples/css/index.js:
     Uncaught AssertionError: false == true
    at ChildProcess.<anonymous> (/Users/awilhelm/dev/duo/test/examples.js:43:9)
matthewmueller commented 10 years ago

Personally, I think locals are a bad idea, but I could be swayed.

I've mentioned this in previous threads but locals offer no insight into where your dependencies are coming from. When you are loading 20-30 modules in your source, there's no syntactical different between remote components and local components, which can get really confusing.

In duo right now you can mimic local components by just requiring the component.json. require('./lib/signup/component.json'), which offers more insight into what your component is.

TBH, I probably wouldn't do that and I'd just opt for require('./lib/signup'), which defaults to index.js or require('./lib/signup/signup.js') for anything other than main.

At the end of the day, it takes about 2 minutes to add a component.json to a local component and open source it. So I don't see the real value.

matthewmueller commented 10 years ago

we could also potentially offer NODE_PATH for those who want it.

@ryanfields to your earlier points.

Relative paths are not consistent across an application. I know that I need widget. I want to require("widget"). Instead, I have to trace my way from the file I'm working in to the location of widget. Maybe it's "../widget" when I'm in my lib directory. Maybe it's "../../src/widget" when I'm in my test directory. Even worse, if widget moves when I'm reorganizing a project, everything blows up.

I think if you go from the root everything gets a lot more consistent, so instead of ../../src/widget, it's something like /src/widget every time. Some people don't like this, but I think it's actually less magical and confusing than what locals is doing.

Duo is supposed to be backward compatible with component.json. That means it should be able to work with existing components that rely on the local and paths entries to resolve some of their dependencies.

This hasn't ever worked in component, so there shouldn't be any components that you'd require that have a locals field. see this issue: https://github.com/componentjs/component/issues/483

beverlycodes commented 10 years ago

@MatthewMueller Two important things to note. Not all of us have the liberty of open-sourcing every component we write. Second, components may not always be intended for use across multiple applications. Duo isn't just offering Components. In giving us a require with CommonJs semantics, Duo is giving us a module system. This module system is great for separating concerns within a single application. Having to create separate repos for every module in an application negates their benefit unless every single module is intended to be used in multiple applications (which is rarely, if ever, the case. Apps will always have modules specific to that app, and their quantity can be significant.)

The challenge with local components is that their physical location now might not be the best physical location later. With remote components, I believe I can decouple myself from their "physical" location by adding the component as a dependency in component.json and requiring it by its repo name (without the Github account prefix). If I suddenly need to use a fork, or if the main repo itself moves to another service (keeping the same name), I don't have to hunt down every last require in my code and update it. With locals, I have no way to insulate myself from having to hunt down and fix requires if the current local organization of my components changes in the future.

If Duo is, at least in part, intended to make it easier to quick-start a project, Duo should also offer ways to mitigate the amount of require statement updates as the application's structure evolves. Node does this gracefully in the way it searches for modules. I don't think Node's resolution strategy makes sense within Duo, but something similar is needed.

If you're just using Duo to include general-use Components, very little of what I'm asking for seems necessary. If you're using Duo as a complete module system, separating all of your app's concerns into appropriate modules, then the handling of relative paths becomes critical. I need a canonical way to refer to my modules. Relative paths can not be canonical. Absolute (a.k.a. root-relative) paths, while perhaps more consistent, are too tightly coupled to an exact component location on disk.

If I can refer to remote modules solely by name by adding them to the dependencies entry in component.json, I should be offered a similar facility for handling components that are on disk. In fact, I should be able to refer to all components solely by name if I want to, and let component.json or some other facility tell Duo how to resolve those components. I get that it's super convenient to just require from github on a whim, but as I get deeper into a project, I get really sick of stuff like require ('components/rsvp.js:/rsvp.js') recurring every time I need Promises in one of my modules. And I'm going to get out in front of any suggestion to just make a "common_requires.js" by stating that doing so leads to an inability to track when dependencies become no longer used.

matthewmueller commented 10 years ago

Yah that makes sense, I think we're just trying to figure out what level of magic there should be with path resolution. I'd like some other people to chime in too:

@yields @ianstormtaylor @stephenmathieson @dominicbarnes

Do you see any benefit of locals vs something like DUO_PATH which is more node-like?

Not all of us have the liberty of open-sourcing every component we write. Second, components may not always be intended for use across multiple applications.

We're absolutely on the same page with this, it's more of a clarity issue for me, when you have multiple possible resolution paths.

I get that it's super convenient to just require from github on a whim, but as I get deeper into a project, I get really sick of stuff like require ('components/rsvp.js:/rsvp.js')

Unrelated, but using a component.json, that can be simplified to require('rsvp.js:rsvp.js'). I'm thinking about supporting the path in the component.json as well. that might get kind of noisy though so we'll see.

timaschew commented 10 years ago

This hasn't ever worked in component, so there shouldn't be any components that you'd require that have a locals field. see this issue: componentjs/component#483

The issue was fixed a half year ago, forgot to close it :)

I've mentioned this in previous threads but locals offer no insight into where your dependencies are coming from. When you are loading 20-30 modules in your source, there's no syntactical different between remote components and local components, which can get really confusing.

IMO this is a weak argument. If you have so many dependencies, then don't use the short syntax to require the modules, for instance instead of require('dom') use require('component/dom')

yields commented 10 years ago

@lancejpollard might have some insight too, i think he's been working with Duo on largish apps a lot lately.

stephenmathieson commented 10 years ago

Do you see any benefit of locals vs something like DUO_PATH which is more node-like?

i never use NODE_PATH. it feels awkward to start apps with $ NODE_PATH=lib node app so that my require()s are a bit prettier. with a bit of investigation, i'm usually able to rely on some 3rd party thing for ./lib/ stuff anyway.

i'd rather see support for locals added, but still don't think it's all that useful. if you're pushing for it to get rid of require('../../../../../../../my-module'), then you're using duo wrong (imo).

beverlycodes commented 10 years ago

For now, I'm just going to use Duo.root-relative paths, as per @MatthewMueller's suggestion. It works but it's unintuitive. Paths starting with / are normally expected to be absolute to the file-system root, not the project root. It also means that all of my local dependencies must be contained in Duo.root or its subtrees, unless I'm willing to fall back to relative "../../.." pathing if I need externally managed local dependencies.

My core complaint here is that Duo doesn't give me any tools to manage local dependency handling, but has very strong opinions on how I should handle local dependencies. That's not maintainable.

sankargorthi commented 10 years ago

The only reason I use local dependencies in component(1) is because depending on custom remotes on unsupported git repository hosts (gitlab or a custom got hosting solution) is really awkward. I'd rather be on github all the time but it's not possible for my company to pay for private hosting.

If remotes were simple to do in duo/component, I would separate out the components to maintain their own lifecycles.

My 2¢

stephenmathieson commented 10 years ago

@sankargorthi yeah duo really needs to support hosts other than github

beverlycodes commented 9 years ago

Closing this. I'm not sure what solution, if any, may eventually come about to bring local components into parity with remote components, but it's unlikely to be this one.