LiveDevMultiBrowser should properly search parent directories for an index.html #10192
Conversation
|
I hadn't signed the CLA. Fixed. |
|
@busykai We're going to make 1 more build for 1.1. Is the benefit vs. risk ratio high enough to consider this fix? |
| @@ -417,7 +417,7 @@ define(function (require, exports, module) { | |||
| // We found no good match | |||
| if (i === -1) { | |||
| // traverse the directory tree up one level | |||
| containingFolder = FileUtils.getDirectoryPath(containingFolder); | |||
| containingFolder = FileUtils.getDirectoryPath(FileUtils.stripTrailingSlash(containingFolder)); | |||
There was a problem hiding this comment.
@dsrw, there's a FileUtils.getBaseName() which should be used here instead of FileUtils.getDirectoryPath() -- it will do the job right.
There was a problem hiding this comment.
@busykai I think getBaseName() just returns the dir name alone, without any preceding path segments. What we want here is the full path of the parent folder. The LivePreview.js code this was copy-pasted from uses a getParentFolder() helper to do this, with a TODO suggesting we should move it into FileUtils. Maybe now is a good time to do that, so it can be shared with the code here.
While we're at it, we may also want to remove LiveDevelopment.js's copy of getFilenameWithoutExtension() which is 100% identical to the one in FileUtils (i.e. the version being called in this copy of the code).
There was a problem hiding this comment.
Of course, eventually we should clean this up even further since almost the entire body of _getInitialDocFromCurrent() is duplicate code (and I'm guessing a bunch of the other code in this file too). It's unclear how long LiveDevelopment.js needs to exist independently, though -- the biggest open question to me is whether we'd need to keep the old implementation around for a long time to support the custom-server use case. A good discussion to start 2015 with, maybe :-)
There was a problem hiding this comment.
Yeah, you're right. I wasn't lucid. :( FileUtils.getBaseName() is not what's needed.
A discussion starter on TODO things for this implementation, including consolidating these two is a due e-mail from us (me and @sebaslv).
|
@redmunds, this PR may require some minor re-work. how much time do we have before the next build? this may not be as much of pressing issue since starting LiveDev from a css is [probably] not the most common use case. even though the issue is there, of course, and it has to be fixed. |
| @@ -102,6 +102,18 @@ define(function (require, exports, module) { | |||
| }); | |||
| }); | |||
|
|
|||
| it("should find an index.html in a parent directory", function () { | |||
There was a problem hiding this comment.
@dsrw, thanks for the test!
|
In the interest of balancing risk/reward before the 1.1 release, I think the best approach would be to copy the I'll do another PR against master that moves Thoughts? |
…e this into FileUtils for 1.2
|
I would much rather prefer a complete change in a single PR. This is not as critical to rush it in. @dsrw, are you up to making the change? Please use |
|
Ah, this is against release, please create a new one against master. |
|
Yeah, I can make the change, but I'm not sure if I'll have docs/tests ready for the FileUtils changes today. As this is a crash bug, I think it would be worth getting it into 1.1, even if it requires later cleanups. |
|
@dsrw Because the timing is so sensitive, we'll finish the rest of that cleanup this morning to make sure this lands in time for 1.1. I'll put up a new PR which includes your first commit to make sure you get credit in the commit history :-) |
…view to use it. Also removed the getFilenameWithExtension helper in favor of FileUtils.getFilenameWithExtension.
|
@peterflynn much appreciated, thank you. I added the proposed cleanups to the PR, but I won't take offence if you'd rather do it yourself. I also fixed a minor error in the FileUtils.getDirectoryPath tests. Is there anything else I can do to help with this? Should I resubmit a squashed version of the change? Resubmit against master? |
Presently, LiveDevMultiBrowser gets stuck in an infinite loop trying to step into parent directories to find an initial index.html if none is present in the current directory. Without the fix, the included test will loop forever.