Monkeypatch ternServer.normalizeFilename so it doesn't break Mac/Linu…

[marcelgerber]

…x absolute paths

Fixes two issues:

• All Extensions/JavaScript Code Hinting tests fail on Linux/Mac (reported by @ficristo in Update Tern & Acorn #11569 (comment))
• On Linux (probably also Mac), Code Hints are not shown immediately, but only after invoking them using Ctrl+Space (reported by @zaggino on Slack)

I could only test this on Linux.

The corresponding Tern "culprit" commit is ternjs/tern@24044a4

[ficristo]

On OSX Extensions \ Javascript Code Hints - JSQuickEdit now pass 🎆
Thank you @marcelgerber

Do you think we should file an issue upstream?

[ficristo]

By the way, how do you discovered this?

And should we replace ecma5 in ecmascript in HintUtils.js?

[zaggino]

ping @marijnh any advice on this?

[marijnh]

Diabling normalizing of filenames will definitely break several aspects of Tern. I couldn't find out, from this issue, why you'd want to do that.

[marcelgerber]

git bisect is my friend. I used my original branch (which contains several Tern updates) to bisect there, then after I found the culprit there, I bisected Tern itself.

Yes, I think we should change the two leftover ecma5 references to ecmascript, so these literals and keywords are considered builtin (and as such, not as highly ranked). Could you raise a PR for that?

I'll file an issue over at Tern, even though I'm not sure this is an issue that many face. I also have a simple fix in mind.

[marcelgerber]

Oh, didn't think you, @marijnh, would actually look at this issue, heh.
So let me explain a bit (I have to admit though that I don't know too much about Tern and how Brackets interacts with it):
We don't specify a projectDir, so Tern assumes /, which also cannot be changed to "" as this will be reverted in Tern.
As such, the paths Tern requests are at least to some extent absolute paths like /home/project/file1.js. Our file system layer resolves this just fine and delivers the file contents to Tern, which also has no problems handling it. But with normalizeFilename applied to the path, our file system layer is handed a path like home/project/file1.js (without the leading slash), which cannot be resolved (it's considered a relative path).

Notice also that everything worked fine until ternjs/tern@24044a4, which was part of our recent Tern (and Acorn) update.

What issues can such a change cause? I could think of requesting the same file two times (once with the absolute and once with the relative path), but that shouldn't cause too much trouble.

The fix I would suggest on your side is that projectDir: "" should be allowed, but let's see what you have to say.

[zaggino]

@marcelgerber just a quick look, isn't it possible to fix function getFile(name, next) { method instead of normalizeFilename ?

[marcelgerber]

Well, the thing is, the leading slash is an indicator whether the path is absolute or relative, so we cannot decide that with the slash removed.

[marijnh]

I think Tern will only pass normalized file names to getFile, so if you get a relative one, you should be able to assume that it's relative to your project dir, not the cwd.

[marcelgerber]

Yes, but the issue is that when an absolute path is normalized, the trailing slash is stripped so it looks like a relative one.

[marijnh]

I understand, but I think you can make your getFile interpretation handle that (as @zaggino also suggested)

[marcelgerber]

@zaggino Could you try adapting the handleTernGetFile method? I wouldn't know how we can differentiate these two types of paths.

[zaggino]

Ok, I'm gonna look at this tomorrow and report back.

[marijnh]

I wouldn't know how we can differentiate these two types of paths.

(That's what I was talking about here )

[zaggino]

@marcelgerber I though about something like this https://github.com/adobe/brackets/compare/zaggino/tern

@marijnh I'd be useful if getFile method could receive also the parent information which is the file from which the name is referenced (like here: https://github.com/ternjs/tern/blob/master/lib/tern.js#L308-L315). srv.options.getFile(name, parent, function(err, text) { would be enough although it'd break the api in the current version. If I had the parent info, it would be easier to pinpoint which file is tern actually requesting.

[zaggino]

Just noticed my branch made 1 test failing on windows and it worked for like only half of the linux tests. This normalizeFilename feature is quite a pain indeed.

[zaggino]

It's configured as no-op here: https://github.com/ternjs/tern/blob/459b6705bada72027fbb922f4097b4dcf0ab305d/lib/tern.js#L24

Why our

ternServer.normalizeFilename = function (name) { return name; };

makes any difference?

[marcelgerber]

https://github.com/ternjs/tern/blob/0bf87473bc8c21f8d37e2ff6157edf9df8bde521/lib/tern.js#L222-L226 is the one we care about. The one you found is only the one that feeds this one :)
Sadly, we cannot really do much with options.normalizeFilename, except for maybe a hack like prepending bracketsHack to every path and then dropping it later on, which is not much better than what we have now.

@zaggino Your solution probably fails in cases like /home/project/home/project/file1.js, where the project dir is /home/project. This is one edge case we definitely have to think about.
In this case, if we would get a request for home/project/file1.js (as I said above, we get both absolute and relative requests), we cannot tell which one it is.

[zaggino]

Ok, understood, your solution has my LGTM then. 👍

[marcelgerber]

Would love to hear from @ficristo and @marijnh, too.
Now that @zaggino has apparently faced similar problems to those I described before, I feel confident that this is an issue that cannot be solved "nicely", at least on our side. Maybe now there's a possibility for a Tern fix?

[ficristo]

If @marijnh could provide an API for us to use would be great.
But I don't have any opinions on the issue itself.

[marijnh]

I stated before that normalization is necessary for Tern to function reliably. If you break it like this, you'll get situations where the same file is referred to by different names (for example when found through a relative import), and the module system will get confused.

I recommended to handle this in getFile, and explained that you don't need additional info, since you know that you get a normalized filename, and that this is relative to the project root.

[zaggino]

I'm afraid that for tern resolving to work properly as intended, we'd have to not use file system root as tern server root but actual require root (not project root, explanation follows).

That brings us to a problem where project root might not be an actual requirejs root (if I open a subdirectory of a project). I can even have multiple requirejs roots in my opened folder structure (every default extension has its own require root https://github.com/adobe/brackets/blob/master/src/utils/ExtensionLoader.js#L172-L173 but usually you open just the brackets project). The only way I would know how to solve this problem is if in the getFile method I'd know, which file is the parent. Then I would be able to traverse the file tree to locate the nearest requirejs root and server file relative from there.

[zaggino]

...continuing my previous babble ...I don't think this is easily solvable (resolving requirejs stuff). If you have a file with a non-relative imports like require('xxx') you will never know 100% from where you should load it. It depends on many things like which html page loaded the file, how the requirejs was configured with basepath, how are the paths set up, etc. So I believe we should do what we do now and that's to disable the file normalization like Marcel did in the PR.

[marcelgerber]

Going for it now. Hope that's ok, but it seems to be fine with @zaggino and @ficristo

[zaggino]

👍

[ficristo]

Yeah, for now I trust our test suite.
Maybe open a new issue to investigate if we can "re-enable" the normalize function?