Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

fix loading of less imports on win, linux and mac #7522

Merged
merged 1 commit into from
Apr 17, 2014
Merged

fix loading of less imports on win, linux and mac #7522

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/utils/ExtensionUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
define(function (require, exports, module) {
"use strict";

var FileSystem = require("filesystem/FileSystem");

/**
* Appends a <style> tag to the document's head.
*
Expand Down Expand Up @@ -66,6 +68,18 @@ define(function (require, exports, module) {
return $link[0];
}

/**
* getModuleUrl returns different urls for win platform
* so that's why we need a different check here
* @see getModuleUrl()
* @param {!string} pathOrUrl that should be checked if it's absolute
* @return {!boolean} returns true if pathOrUrl is absolute url on win platform
* or when it's absolute path on other platforms
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment needs a reference to hack done in getModuleUrl() to help keep these functions in sync.

Also needs jsdoc info for @param and @return.

function isAbsolutePathOrUrl(pathOrUrl) {
return brackets.platform === "win" ? PathUtils.isAbsoluteUrl(pathOrUrl) : FileSystem.isAbsolutePath(pathOrUrl);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zaggino I don't like the idea of this solution because I think it sets a bad precedent -- pretty soon we'll have to do this everywhere.

The FileSystem.isAbsolutePath() check does not seem to be safe. All it does is check to see if first char is a forward slash (fullPath[0] === "/") which will cause problems if someone passes in a web site root-relative path of something like "/styles/my.less".

What code is failing? Maybe that code should convert the local file path to a url instead.

cc @peterflynn

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right this is a bad solution ... here's the problematic code (line 140):

function getModuleUrl(module, path) {
        var url = encodeURI(getModulePath(module, path));

        // On Windows, $.get() fails if the url is a full pathname. To work around this,
        // prepend "file:///". On the Mac, $.get() works fine if the url is a full pathname,
        // but *doesn't* work if it is prepended with "file://". Go figure.
        // However, the prefix "file://localhost" does work.
        if (brackets.platform === "win" && url.indexOf(":") !== -1) {
            url = "file:///" + url;
        }

        return url;
    }
``

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@redmunds The FileSystem APIs all only work with paths -- if someone passes in a URL instead, they will all fail. I don't think we should change this one API to accept URLs when none of the other related APIs do.

Where is the path originating from in this case? It seems like it's part of the Require module object. If Require has a feature/bug where it sometimes passes file://... URLs and sometimes passes absolute Linux/Mac system paths, then it seems appropriate to me to place a workaround specific to that issue localized in this one bit of code that actually interacts with those Require APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This results in url being different for win vs others so different check is also needed depending on the platform...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, @zaggino sorry, your replies didn't show up until I reloaded the page. So tt seems like our own getModuleUrl() fully explains the funkiness both cases -- not a Require bug.

In the loadFile() case we could just do the absolute check before converting getModulePath() to a URL... but in the loadStyleSheet() -> parseLessCode() case we're going off of the URL fed into $.get() (this.url), which has to have the funkiness... so it does seem like the cleanest solution overall is to just have an absolute-checking utility that handles both cases...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterflynn

I don't think we should change this one API...

I wasn't suggesting that API should be changed, just not used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the check a bit according to getModuleUrl() and fixed the comment, should be much more clear now.


/**
* Parses LESS code and returns a promise that resolves with plain CSS code.
*
Expand All @@ -91,7 +105,7 @@ define(function (require, exports, module) {
rootpath: dir
};

if (PathUtils.isAbsoluteUrl(url)) {
if (isAbsolutePathOrUrl(url)) {
options.currentFileInfo = {
currentDirectory: dir,
entryPath: dir,
Expand Down Expand Up @@ -161,7 +175,7 @@ define(function (require, exports, module) {
* @return {!$.Promise} A promise object that is resolved with the contents of the requested file
**/
function loadFile(module, path) {
var url = PathUtils.isAbsoluteUrl(path) ? path : getModuleUrl(module, path),
var url = isAbsolutePathOrUrl(path) ? path : getModuleUrl(module, path),
promise = $.get(url);

return promise;
Expand Down