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

Test requires sibling projects using specific file names or it fails #73

Open
rxaviers opened this issue Oct 7, 2014 · 8 comments · May be fixed by #74
Open

Test requires sibling projects using specific file names or it fails #73

rxaviers opened this issue Oct 7, 2014 · 8 comments · May be fixed by #74

Comments

@rxaviers
Copy link

rxaviers commented Oct 7, 2014

See PR #74

Test requires installing sibling projects (i.e., it leaks files outside of its root) with specific names or it fails (e.g., if you name your local ecma402 directory my-ecma402, tests will fail).

The suggestion is to use AMD paths mappings, which is more a flexible approach (and less error prone).

rxaviers added a commit to rxaviers/ecma402 that referenced this issue Oct 7, 2014
@rxaviers rxaviers linked a pull request Oct 7, 2014 that will close this issue
4 tasks
rxaviers added a commit to rxaviers/ecma402 that referenced this issue Oct 7, 2014
@cjolif
Copy link
Contributor

cjolif commented Oct 8, 2014

@rxaviers I understand your concern here. However this structure outside of the root is common to all our ibm-js projects in order to avoid projects duplication and works with the overall build system. As far as I know even if not ideal the default paths are working (I mean if you do bower install everything should went fine). And obviously if one wants to install the dependencies differently he can always do using AMD mapping himself? So we should be ok with what we have?

@rxaviers
Copy link
Author

rxaviers commented Oct 8, 2014

So we should be ok with what we have?

  1. Except for the cldr paths that are currently relative and need to be changed to allow AMD path mapping (e.g., from ../cldr/sup... to cldr/sup...). I can reduce this PR to that.
  2. Unless your ibm-js projects use different versions of the same dependency.

@cjolif
Copy link
Contributor

cjolif commented Oct 8, 2014

  1. Yes for cldr, as this is a pre-requesite for using cldr-data I guess.
  2. Yep, this is a constraint we indeed have to deal with

@clmath
Copy link
Member

clmath commented Oct 31, 2014

For 1) my testing shows that there is no need for a module id to be absolute in order to use paths configuration in requirejs. You can use the following configuration:

requirejs.config({
    paths: {
        "ecma402/cldr": "any/path/to/cldr"
    }
}):

So I am not sure this PR still make sense. Obviously, when we have a solution using cldr-data, we will have to update the path to cldr data so no additional configuration is required but in the meantime I don't see a reason to change that.

@rxaviers
Copy link
Author

I'm ok with whatever preference you like here. Although, this is not what I commonly see elsewhere.

In my opinion, libraries should make decisions targeting developers convenience. For example, the simplification of their configuration files.

requirejs.config({
    paths: {
        "cldr": "any/path/to/cldr"
    }
}):

I don't think we want advocate the below.

requirejs.config({
    paths: {
        "ecma402/cldr": "any/path/to/cldr",
        "globalize/cldr": "any/path/to/cldr",
        "foo/cldr": "any/path/to/cldr",
        ...
    }
}):

@cjolif
Copy link
Contributor

cjolif commented Nov 13, 2014

@rxaviers I think we agree with you we just say let's wait for the common CLDR problem (cld-data) being solved first?

@rxaviers
Copy link
Author

Ok, so we're going to land this together with #66 (and not before as I've initially proposed).

@cjolif
Copy link
Contributor

cjolif commented Nov 18, 2014

sounds good.

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

Successfully merging a pull request may close this issue.

3 participants