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

dots (.) in keys for the config.paths breaks lookup for later non-dotted final file #123

Open
metatoaster opened this issue Oct 15, 2016 · 3 comments

Comments

@metatoaster
Copy link

metatoaster commented Oct 15, 2016

Firstly I had this test done in requirejs 2.3.2, requirejs-text 2.0.12, (later I have a test for the latest version (requirejs-text 2.0.15))

var requirejsOptions = ({"paths": { 
  'nodot/target': '/srv/tmp/nodot/target',
  'dot.d/target': '/srv/tmp/dot.d/target',
  'nodot/extra.target': '/srv/tmp/nodot/extra.target',
  'dot.d/extra.target': '/srv/tmp/dot.d/extra.target',
}})
requirejs.config(requirejsOptions);

The above configuration is set up to effectively append /srv/tmp/ to the affected paths to illustrate the issue at hand; the test matrix I used:

require([
  'text!nodot/target',             // pass /srv/tmp/nodot/target
  'text!nodot/target.ext',         // pass /srv/tmp/nodot/target.ext
  'text!nodot/target/file',        // pass /srv/tmp/nodot/target/file
                                   // ____ note missing trailing dot below
  'text!nodot/target/file.',       // FAIL /srv/tmp/nodot/target/file
  'text!dot.d/target',             // FAIL ./dot.d/target
  'text!dot.d/target.ext',         // pass /srv/tmp/dot.d/target.ext
  'text!dot.d/target/file',        // FAIL ./dot.d/target/file
  'text!dot.d/target/file.',       // pass /srv/tmp/dot.d/target/file.
  'text!nodot/extra.target',       // FAIL ./nodot/extra.target
  'text!nodot/extra.target.ext',   // pass /srv/tmp/nodot/extra.target.ext
  'text!nodot/extra.target/file',  // FAIL ./nodot/extra.target/file
  'text!nodot/extra.target/file.', // pass /srv/tmp/nodot/extra.target/file.
  'text!dot.d/extra.target',       // FAIL ./dot.d/extra.target
  'text!dot.d/extra.target.ext',   // pass /srv/tmp/dot.d/extra.target.ext
  'text!dot.d/extra.target/file',  // FAIL ./dot.d/extra.target/file
  'text!dot.d/extra.target/file.', // pass /srv/tmp/dot.d/extra.target/file.
], function() {
});

The above is somewhat closer to expected behaviour, where the passing tests have accessed urls with /srv/tmp/ prepended as configured.

With 2.0.15, same configuration:

require([
                                   // ____ note all missing trailing dot below
  'text!nodot/target',             // pass /srv/tmp/nodot/target
  'text!nodot/target.ext',         // pass /srv/tmp/nodot/target.ext
  'text!nodot/target/file',        // pass /srv/tmp/nodot/target/file
  'text!nodot/target/file.',       // FAIL /srv/tmp/nodot/target/file
  'text!dot.d/target',             // FAIL ./dot.d/target
  'text!dot.d/target.ext',         // pass /srv/tmp/dot.d/target.ext
  'text!dot.d/target/file',        // FAIL ./dot.d/target/file
  'text!dot.d/target/file.',       // FAIL ./dot.d/target/file
  'text!nodot/extra.target',       // FAIL ./nodot/extra.target
  'text!nodot/extra.target.ext',   // pass /srv/tmp/nodot/extra.target.ext
  'text!nodot/extra.target/file',  // FAIL ./nodot/extra.target/file
  'text!nodot/extra.target/file.', // FAIL ./nodot/extra.target/file
  'text!dot.d/extra.target',       // FAIL ./dot.d/extra.target
  'text!dot.d/extra.target.ext',   // pass /srv/tmp/dot.d/extra.target.ext
  'text!dot.d/extra.target/file',  // FAIL ./dot.d/extra.target/file
  'text!dot.d/extra.target/file.', // FAIL ./dot.d/extra.target/file
], function() {
});

If I could get an explanation on how or why a . character in a directory could just later stop a proper file lookup that do not have a filename extension it would be great. Also trailing dots, now that the latest version managed to consume and ignore that completely.

Further to the above, there are a couple more behaviours that I am unsure of (using same configuration as above), they are marked with ????

require([
  'text!nodot/target.d/file', // ???? /srv/tmp/nodot/target.d/file
  'text!nodot/targetd/file',  // ???? ./nodot/targetd/file
], function() {
});

The first one is especially puzzling since it basically treat .d/file as a filename extension to nodot/target, even though file is really a file living under the nodot/target.d directory, which is tracked ambiguously through the configuration.

Finally, for completion sake, doing this directly without the plugin (and account for the automatic .js filename extension appending), this is what I saw.

require([
  'nodot/target',             // pass /srv/tmp/nodot/target.js
  'nodot/target.ext',         // ???? ./nunja/nodot/target/ext.js
  'nodot/target/file',        // pass /srv/tmp/nodot/target/file.js
  'nodot/target/file.',       // pass /srv/tmp/nodot/target/file..js
  'dot.d/target',             // pass /srv/tmp/dot.d/target.js
  'dot.d/target.ext',         // ???? ./dot.d/target.ext.js
  'dot.d/target/file',        // pass /srv/tmp/dot.d/target/file.js
  'dot.d/target/file.',       // pass /srv/tmp/dot.d/target/file..js
  'nodot/extra.target',       // pass /srv/tmp/nodot/extra.target.js
  'nodot/extra.target.ext',   // ???? ./nodot/extra.target.ext.js
  'nodot/extra.target/file',  // pass /srv/tmp/nodot/extra.target/file.js
  'nodot/extra.target/file.', // pass /srv/tmp/nodot/extra.target/file..js
  'dot.d/extra.target',       // pass /srv/tmp/dot.d/extra.target.js 
  'dot.d/extra.target.ext',   // ???? ./dot.d/extra.target.ext
  'dot.d/extra.target/file',  // pass /srv/tmp/dot.d/extra.target/file.js 
  'dot.d/extra.target/file.', // pass /srv/tmp/dot.d/extra.target/file..js 
], function() {
});

require([
  'nodot/target.d/file', // ???? ./nodot/target.d/file.js
  'nodot/targetd/file',  // ???? ./nodot/targetd/file.js
], function() {
});
@jrburke
Copy link
Member

jrburke commented Oct 15, 2016

I expect the problem is mostly in the parseName function. The logic is a bit naive and was really focused on the module/name.ext case. I expect looking at improving that method would fix some of the issues. I still think there is a balance to be had for implementation size vs trying to catch all cases. For example, if trying to detect names that just end with a ., if that adds too much to the implementation size, it may not be worth it.

@metatoaster
Copy link
Author

metatoaster commented Oct 16, 2016

If that is the case, a possible fix is to ensure that for the statement index = name.lastIndexOf("."),, the value is only set if it is greater than the value of name.lastIndexOf("/"), as having a ext that includes any / (typical character for separator between directories) is pretty nonsensical. Also don't actually count the . if it occur at the end of the string. This definition of variables for the parseName function:

            var modName, ext, temp,
                strip = false,
                dot = name[name.length - 1] === "." ? -1 : name.lastIndexOf("."),
                index = name.lastIndexOf("/") > dot ? -1 : dot,
                isRelative = name.indexOf('./') === 0 ||
                             name.indexOf('../') === 0;

Seems to fix the first set of cases I got

  'text!nodot/target',             // pass /srv/tmp/nodot/target
  'text!nodot/target.ext',         // pass /srv/tmp/nodot/target.ext
  'text!nodot/target/file',        // pass /srv/tmp/nodot/target/file
  'text!nodot/target/file.',       // pass /srv/tmp/nodot/target/file.

However the remaining cases still don't work, so I guess something else is at play. That said this fixed the apparent regression that was introduced in 2.0.13

@LukeOwlclaw
Copy link

LukeOwlclaw commented Mar 1, 2018

With version 2.3.5 would this work in nameToUrl (around line 1648) :

if (!(moduleName in config.paths) && req.jsExtRegExp.test(moduleName)) {

instead of

if (req.jsExtRegExp.test(moduleName)) {

It makes sure that defined paths have precedence.

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