Open T1mL3arn opened 1 year ago
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://" 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://" or "https://". Is there any way it could have started with anything else?
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:
path
attributePretty 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);
}
}
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...
I guess the code before refactoring
commit does not have the issue?
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.
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);
}
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
.
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 :disappointed: ) for JS target, so it is kind of a gray area.
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.
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
path
if the path doesn't exist in file system - this way path
works the same as name
, orname
e.g. url
or uri
; deprecate name
attributePeople 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
oruri
; deprecatename
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.
This PR allows this syntax
to include the library in
index.html
.Without this PR, the same can be achieved using
name
attributebut I believe
path
attribute suits better.