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

2.1.12/13 issue with multiple modules and ".." #1131

Closed
jbedard opened this Issue May 27, 2014 · 8 comments

Comments

Projects
None yet
2 participants
@jbedard

jbedard commented May 27, 2014

It looks like one of the 2.1.12/13 fixes broke the use of ".." along with multiple modules. The better normalizing of ".." is great (I've worked around it in the past), but it seems like it is currently also triming modules?

Here's a snippet that worked in 2.1.11 but no longer works in 2.1.13.

<script>
      define('modA', [], function() {
        var CONFIG = "foo";

        return {
          load: function(name, req, onload, config) {
            req([name + "-" + CONFIG], onload, onload.error);
          }
        }
      });

      define('modB', [], function() {
        return {
          load: function(name, req, onload, config) {
            document.body.appendChild( document.createTextNode(name) );
          }
        }
      });

      require(['modA!modB!no-dotdot']);
      require(['modA!modB!../one-dotdot']);
      require(['modA!modB!../../two-dotdot-is-broken']);
</script>
@jrburke

This comment has been minimized.

Show comment
Hide comment
@jrburke

jrburke May 27, 2014

Member

Thanks for the sample -- could you list out how the files are arranged on disk, so I can be sure I can locally reproduce correctly?

Member

jrburke commented May 27, 2014

Thanks for the sample -- could you list out how the files are arranged on disk, so I can be sure I can locally reproduce correctly?

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard May 27, 2014

Here an example closer to my actual use case with a directory structure.

app <-- baseUrl
  + styles
     + style-a.css
     + style-b.css
  + js
     + modA.js
     + modB.js
     + main.js
     + widgets
         + test.js

test.js

define(["modA!./modB!../../style"])

Where modA decides if style-a or style-b should be used and modB loads the css file. But modB isn't even called and style-*.js is loaded instead. If main.js loads the style then it is fine I assume because there would be only one "..".

jbedard commented May 27, 2014

Here an example closer to my actual use case with a directory structure.

app <-- baseUrl
  + styles
     + style-a.css
     + style-b.css
  + js
     + modA.js
     + modB.js
     + main.js
     + widgets
         + test.js

test.js

define(["modA!./modB!../../style"])

Where modA decides if style-a or style-b should be used and modB loads the css file. But modB isn't even called and style-*.js is loaded instead. If main.js loads the style then it is fine I assume because there would be only one "..".

@jrburke

This comment has been minimized.

Show comment
Hide comment
@jrburke

jrburke May 27, 2014

Member

Ah nevermind, I see the point of the example now, the loader plugin ID interactions.

Member

jrburke commented May 27, 2014

Ah nevermind, I see the point of the example now, the loader plugin ID interactions.

@jrburke

This comment has been minimized.

Show comment
Hide comment
@jrburke

jrburke May 27, 2014

Member

Oops, missed your other comment, thanks, looking into it now!

Member

jrburke commented May 27, 2014

Oops, missed your other comment, thanks, looking into it now!

@jbedard jbedard closed this May 27, 2014

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard May 27, 2014

Oops, wrong button :|

jbedard commented May 27, 2014

Oops, wrong button :|

@jbedard jbedard reopened this May 27, 2014

@jrburke

This comment has been minimized.

Show comment
Hide comment
@jrburke

jrburke May 27, 2014

Member

This one is at the intersection of the lax, buggy ID behavior from before and how loader plugin resource IDs are considered.

In general, for a loader plugin dependency ID like pluginId!resourceId, the loader just does one separation on the first '!', normalized pluginId, loads the plugin, then asks the plugin how it should normalize the resourceId.

If the plugin does not implement a normalize method, then the loader will assume it is a plain JS module ID and normalize it according to those rules, which just does the dot trimming and making the ID absolute. It does not consider that the resourceId could contain another plugin ID.

Due to the previous weakness in ID resolution before 2.1.12, it was a happy coincidence that the trimDots allowed the secondary plugin ID reference to pass through.

What sort of action to take to fix:

  1. If you need something to work immediately, you can implement a normalize method on the plugin like below, because resource IDs are very opaque to the loader on purpose.
      define('modA', [], function() {
        var CONFIG = "foo";

        return {
          normalize: function(name, normalize) {
            if (name.indexOf('!') !== -1) {
              return name;
            } else {
              return normalize(name);
            }
          },
          load: function(name, req, onload, config) {
            req([name + "-" + CONFIG], onload, onload.error);
          }
        }
      });
  1. However, a good question to raise is, if the loader plugin does not implement normalize, why does the loader's default resourceId normalization does not also include the normalization logic that looks for a loader plugin ID, and just does the JS ID work.

I think this is the a correct question to wonder. It does complicate the logic a bit more, but I believe is the correct overall fix. By fixing the other normalization issue, it uncovered this weakness.

I think the lower level fix is the best long term strategy, but I am still investigating how big of a change that is. If it looks too big, then I may roll back the normalization changes I did in 2.1.12 and 2.1.13 and then target a fix for 2.2. Although I would prefer to just do a complete fix now instead of kicking the other normalization issue forward.

Member

jrburke commented May 27, 2014

This one is at the intersection of the lax, buggy ID behavior from before and how loader plugin resource IDs are considered.

In general, for a loader plugin dependency ID like pluginId!resourceId, the loader just does one separation on the first '!', normalized pluginId, loads the plugin, then asks the plugin how it should normalize the resourceId.

If the plugin does not implement a normalize method, then the loader will assume it is a plain JS module ID and normalize it according to those rules, which just does the dot trimming and making the ID absolute. It does not consider that the resourceId could contain another plugin ID.

Due to the previous weakness in ID resolution before 2.1.12, it was a happy coincidence that the trimDots allowed the secondary plugin ID reference to pass through.

What sort of action to take to fix:

  1. If you need something to work immediately, you can implement a normalize method on the plugin like below, because resource IDs are very opaque to the loader on purpose.
      define('modA', [], function() {
        var CONFIG = "foo";

        return {
          normalize: function(name, normalize) {
            if (name.indexOf('!') !== -1) {
              return name;
            } else {
              return normalize(name);
            }
          },
          load: function(name, req, onload, config) {
            req([name + "-" + CONFIG], onload, onload.error);
          }
        }
      });
  1. However, a good question to raise is, if the loader plugin does not implement normalize, why does the loader's default resourceId normalization does not also include the normalization logic that looks for a loader plugin ID, and just does the JS ID work.

I think this is the a correct question to wonder. It does complicate the logic a bit more, but I believe is the correct overall fix. By fixing the other normalization issue, it uncovered this weakness.

I think the lower level fix is the best long term strategy, but I am still investigating how big of a change that is. If it looks too big, then I may roll back the normalization changes I did in 2.1.12 and 2.1.13 and then target a fix for 2.2. Although I would prefer to just do a complete fix now instead of kicking the other normalization issue forward.

@jrburke

This comment has been minimized.

Show comment
Hide comment
@jrburke

jrburke May 27, 2014

Member

I added in a fix. It is not a completely generic fix (using #1132 to track that), but it should be in line with what was allowed in previous 2.1.x releases.

If you can try it out: master require.js and let me know if that resolves the issue for you, I will look into spinning out a 2.1.14 release soon.

Member

jrburke commented May 27, 2014

I added in a fix. It is not a completely generic fix (using #1132 to track that), but it should be in line with what was allowed in previous 2.1.x releases.

If you can try it out: master require.js and let me know if that resolves the issue for you, I will look into spinning out a 2.1.14 release soon.

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard May 27, 2014

That fixes it. Thanks for the quick response!

jbedard commented May 27, 2014

That fixes it. Thanks for the quick response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment