-
Notifications
You must be signed in to change notification settings - Fork 117
Support "local" directive in component.json. #372
Conversation
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. |
why do you need edit: have you seen this: https://github.com/poying/duo-locals |
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 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. |
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. |
Ah, I think I see a problem. I need to fix the exists call. co-fs exists is weird... |
} | ||
}); | ||
ret = localPaths.filter(function (path) { | ||
return fs.exists(join(path, dep)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there another way to handle this? fs.exists()
is considered an anti-pattern :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is asserting whether or not a file exists an antipattern?
If I have a list of search paths in which a file might be found, and need to return at least one path where that file was found, I'm not sure how else I'd resolve that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephenmathieson I think that's just FUD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephenmathieson Did you mean fs.exists specifically? I switched over to co-exists since I noticed Duo is already using that.
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. |
👍 👍 👍 |
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.
With node 0.10.33 I got a totally different error, but again only for the first time.
|
Personally, I think I've mentioned this in previous threads but In duo right now you can mimic local components by just requiring the TBH, I probably wouldn't do that and I'd just opt for At the end of the day, it takes about 2 minutes to add a |
we could also potentially offer @RyanFields to your earlier points.
I think if you go from the root everything gets a lot more consistent, so instead of
This hasn't ever worked in component, so there shouldn't be any components that you'd require that have a |
@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 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 |
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
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.
Unrelated, but using a component.json, that can be simplified to |
The issue was fixed a half year ago, forgot to close it :)
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 |
@lancejpollard might have some insight too, i think he's been working with Duo on largish apps a lot lately. |
i never use 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 |
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 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. |
The only reason I use local dependencies in If remotes were simple to do in duo/component, I would separate out the components to maintain their own lifecycles. My 2¢ |
@sankargorthi yeah duo really needs to support hosts other than github |
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. |
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.