[js] allow to use remote js dependecies like https://unpkg.com/react.js in Project.xml

[T1mL3arn]

This PR allows this syntax

<dependency path="https://unpkg.com/[email protected]/simplepeer.min.js" />

to include the library in index.html.

Without this PR, the same can be achieved using name attribute

<dependency name="https://unpkg.com/[email protected]/simplepeer.min.js" />

but I believe path attribute suits better.

[player-03]

I like the idea, but I wonder if it's time to switch to regex. startsWith("http") isn't foolproof.

context.linkedLibraries = [];

//Matches strings that start with "http:https://" or "https://" and end with ".js".
var urlMatcher:EReg = ~/^https?:\/\/.+\.js$/i;

for (dependency in project.dependencies)
{
	if (dependency.embed && !npm)
	{
		continue;
	}

	if (urlMatcher.match(dependency.name) || urlMatcher.match(dependency.path))
	{
		context.linkedLibraries.push(urlMatcher.matched(0));
	}
	else if (StringTools.endsWith(dependency.path, ".js") && FileSystem.exists(dependency.path))
	{
		var name = Path.withoutDirectory(dependency.path);

		context.linkedLibraries.push("./" + dependencyPath + "/" + name);
		System.copyIfNewer(dependency.path, Path.combine(destination, Path.combine(dependencyPath, name)));
	}
}

(My personal preference is to reduce nesting where possible, hence the continue.)

The thing about this implementation is that dependency.name now has to start with "http:https://" or "https://". Is there any way it could have started with anything else?

[T1mL3arn]

Is there any way it could have started with anything else?

Yes, in github a found enough examples where name attribute is used like this:

<dependency name="js/some-third-party-lib.js" />

So for backward compat name attr code should be untouched. It looks like name was meant for any js dependency, but attribute name is kinda confusing.

There are basically two options:

• don't touch the code, just update js lime docs
• allow to use any js in path attribute

[player-03]

Pretty sure the "name" and "path" attributes exist for the benefit of other targets. Perhaps for JS we could treat them as synonyms?

context.linkedLibraries = [];

for (dependency in project.dependencies)
{
	if (dependency.embed && !npm)
	{
		continue;
	}

	var path:String;

	if (StringTools.endsWith(dependency.name, ".js"))
	{
		path = dependency.name;
	}
	else if (StringTools.endsWith(dependency.path, ".js"))
	{
		path = dependency.path;
	}
	else
	{
		continue;
	}

	if (FileSystem.exists(path))
	{
		var name = Path.withoutDirectory(path);

		context.linkedLibraries.push("./" + dependencyPath + "/" + name);
		System.copyIfNewer(dependency.path, Path.combine(destination, Path.combine(dependencyPath, name)));
	}
	else
	{
		context.linkedLibraries.push(path);
	}
}

[player-03]

What are the odds that someone put in a name (e.g., <dependency name="js/some-third-party-lib.js" />) that matches a file on their local system, but they don't actually want to use that local file? Hopefully not likely, but this is technically a difference in behavior...

[T1mL3arn]

I guess the code before refactoring commit does not have the issue?

[player-03]

Yeah, and there are several other ways not to do that. I'm just trying to figure out the most appropriate option.

If our only goal was to give the user the choice between "local file" and "resolvable url," we'd close this pull request and leave the code as-is. If our only goal was to reduce confusion stemming from attribute names, we'd make them synonyms. I personally prefer the latter, but Lime is pretty big on backwards compatibility, so I'm trying to decide exactly how backwards-compatible the "synonyms" option is.

[T1mL3arn]

how backwards-compatible the "synonyms" option is

i believe this has desired compatibility:

if (StringTools.endsWith(dependency.path, ".js") && FileSystem.exists(dependency.path))
{
  var name = Path.withoutDirectory(dependency.path);
  
  context.linkedLibraries.push("./" + dependencyPath + "/" + name);
  System.copyIfNewer(dependency.path, Path.combine(destination, Path.combine(dependencyPath, name)));
}
else if (StringTools.endsWith(dependency.path, '.js'))
{
  context.linkedLibraries.push(dependency.path);
}
else if (StringTools.endsWith(dependency.name, ".js"))
{
  context.linkedLibraries.push(dependency.name);
}

[player-03]

Yes, but then the attributes aren't synonymous. There are lots of perfect backwards-compatible options, but by definition, none of them make name equivalent to path.

[T1mL3arn]

Yes, but then the attributes aren't synonymous.

Why is that? They both do the same - user strings like js/some-lib.js become <script type="text/javascript" src="js/some-lib.js"></script>. Though there can be a project with dependency with both name and path, but in fact, they were never documented (they actually could be in a changelog or in a blog post which nobody read 😞 ) for JS target, so it is kind of a gray area.

[player-03]

They both do the same

Not if the file exists locally. path offers extra behavior for local files that name doesn't.

To be synonyms, they have to be completely interchangeable in all cases. And the question is, do we want them to be completely interchangeable, or should there be a difference?

Personally, I don't think this distinction makes sense. If you asked me to guess which of name or path could refer to a local file and which couldn't, I wouldn't have been able to tell you. But I'm not sure what the solution is, either.

[T1mL3arn]

the question is, do we want them to be completely interchangeable, or should there be a difference?

If that is the question then of course there is a difference.

name is sort of a bad attribute name for what it is doing. People on github are using it like path, but they copy js files with help of <template> .

what the solution is

• add new behavior for path if the path doesn't exist in file system - this way path works the same as name, or
• new attribute with better name to be a synonym for name e.g. url or uri; deprecate name attribute

[player-03]

People on github are using it like path, but they copy js files with help of <template> .

Well that's a bit redundant. But there's no good way to stop people from writing redundant xml. Especially not now that they've already done it and published the code. We just have to keep supporting it until version 9.0.0 at the earliest.

• new attribute with better name to be a synonym for name e.g. url or uri; deprecate name attribute

Most targets can use the <dependency /> tag, so we want to avoid adding JS-specific attributes. Especially attributes that would be easy to confuse with path.

I can't see any good solutions here, but "add new behavior for path" seems like the least bad at the moment.