Skip to content
This repository was archived by the owner on Oct 9, 2020. It is now read-only.

Enhancement: Include "canonicalPath" in external dependencies for SFX bundles. (utils.js->getAlias) #470

Closed
typhonrt opened this issue Jan 16, 2016 · 2 comments

Comments

@typhonrt
Copy link
Contributor

Greets Guy et al,

So, in my further library writing regarding backbone-parse-es6 to solve a serialization issue I now have to override a Parse library file (ParseObject). This new class BackboneParseObject imports a file from parse/lib/browser/encode.js with parse being aliased to npm:parse. When creating a SFX bundle for backbone-parse-es6 which excludes external dependencies including Parse I use this 'meta' config.

In particular I mark as external dependencies anything under this path: "parse/lib/browser/*": { "build": false }

Currently in utils.js -> getAlias (outdated / fix applied!) only the canonical name of the module is resolved which is just parse and the path is dropped. This is no good and causes things to break for the SFX bundle. For instance for the AMD bundle it looks like this:

(function(factory) {
  define(["underscore","parse","jquery","parse"], factory);
});

when it really should be this:

(function(factory) {
  define(["underscore","parse","jquery","parse/lib/browser/encode"], factory);
});

The potential issue is in utils.js -> getAlias and in particular right here.

Here is a new getAlias with my changes to support canonical paths in addition to canonical name substitution / matching:

exports.getAlias = getAlias
function getAlias(loader, canonicalName) {
  var pluginIndex = loader.pluginFirst ? canonicalName.indexOf('!') : canonicalName.lastIndexOf('!');

  if (pluginIndex != -1)
    return getAlias(loader, canonicalName.substr(0, pluginIndex)) + '!' + getAlias(loader, canonicalName.substr(pluginIndex + 1));

  if (canonicalName.match(/\#[\:\{\?]/))
    throw new Error('Get alias not implemented for conditional name "' + canonicalName + '". Remove the conditional exclusion, or post a SystemJS Builder bug if needed!');

  var canonicalPath;

  if (canonicalName.indexOf('/') >= 0)
    canonicalPath = canonicalName.substr(canonicalName.indexOf('/'), canonicalName.length);

  var bestAlias;

  function getBestAlias(mapped) {
    return canonicalName.substr(0, mapped.length) == mapped &&
        (canonicalName.length == mapped.length || canonicalName[mapped.length] == '/');
  }

  Object.keys(loader.map).forEach(function(alias) {
    if (getBestAlias(loader.map[alias]))
    {
      bestAlias = alias;

      if (canonicalPath)
        bestAlias += canonicalPath;
    }
  });

  if (bestAlias)
    return bestAlias;

  return canonicalName;
}

The above change does the trick and properly exports the aliased name + canonical path correctly. I don't have any visibility into if such a change would cause problems. If so though perhaps an additional meta boolean buildIncludePath could be added and when setting to true ala: "parse/lib/browser/*": { "build": false, "buildIncludePath": true } then the canonical path is included.

I can create a PR, but just want to get your input on the issue first.
Thanks...

@guybedford
Copy link
Member

@typhonrt thanks, this is definitely a bug. A PR to getAlias would be really great.

@typhonrt
Copy link
Contributor Author

Excellent. The associated PR: #483

Do let me know about #478 too as it's related to this change.

@guybedford guybedford removed the bug label Jan 24, 2016
guybedford added a commit that referenced this issue Jan 24, 2016
Fix for SystemJS Builder Issue #470 adding canonical path resolution.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants