componentjs / builder2.js

builder for component
50 stars 20 forks source link

fix case-sensitive deps not resolving #85

Closed tetsuo closed 9 years ago

tetsuo commented 9 years ago

Scripts#lookup starts by lowercasing the target name before doing its job but this causes some trouble if you need to require a file name with some mixed case letters in it.

The trouble is that this doesn't resolve to anything:

require('repo/FileName')

Now, one way of working around this problem is to lowercase everything first, which in fact what builder is doing right now and most certainly doesn't bring the desired outcome from the developer's perspective. And it doesn't work either, because resolver.js doesn't know that.

A better option is to lowercase only user and repo fragments (this patch)

Since we know user and repo are case insensitive, and must be case insensitive; we can safely translate the reference to lowercase and keep the rest untouched.

timaschew commented 9 years ago

did you see this: https://github.com/componentjs/guide/blob/master/creating-components/best-practices.md#only-use-lowercased-letters-in-files-and-component-names and this https://github.com/componentjs/spec/blob/master/component.json/specifications.md#name ?

tetsuo commented 9 years ago

Yeah sure. I know using lowercase letters is the preferred way, and to me enforcing this rule is no problem at all. But this fix doesn't break anything, passes all the tests. Making some mixed-case minded people happy will do no harm to the current ecosystem.

Besides, think of a situation where you want to support both browserify and component builds. Browserify works absolutely fine with mixed-case letters, but component doesn't. And now if you are stuck with a giant lib that has some crazy letters in it, you will most probably drop the component support rather than forking up this beast and maintain component compatibility.

As I said, this will do no harm and actually make some good.

timaschew commented 9 years ago

okay, good point, thanks. Have no time this week, but I will check it next week

tetsuo commented 9 years ago

:+1:

tetsuo commented 9 years ago

@timaschew Fixing another issue (infamously @raynos bug https://github.com/componentjs/builder2.js/blob/625f3a7f0f5a7f60239ef9b0b7db43c59d31caa3/lib/builders/scripts.js#L323) right now.

Please do not merge this, not yet, even if you intend to :) I'll send another request

timaschew commented 9 years ago

Please do not merge this, not yet, even if you intend to :) I'll send another request

So this can be closed?

tetsuo commented 9 years ago

@timaschew have you seen this one? https://github.com/componentjs/builder2.js/pull/86

tetsuo commented 9 years ago

if #86 is ok i'll merge this with that one

tetsuo commented 9 years ago

Closing this. See #87