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

Use the global RequireJS config for ExtensionLoader. Fixes #1087 #12041

Merged
merged 3 commits into from
Aug 21, 2016

Conversation

petetnt
Copy link
Collaborator

@petetnt petetnt commented Jan 2, 2016

Brackets has a long running (minor) issue #1087, where the global RequireJS path configuration is not being re-used but instead repeated in ExtensionLoader.js.

This PR fixes the issue by creating a simple wrapper API around the semi-private (see James Burkes comment here) requirejs.s.contexts._.config and later converts them to absolute paths in ExtensionLoader.js.

This same method can also be used to retrieve global modules with other modules that aren't run in the same scope as the rest of Brackets, such as WebWorkers and Node instances. For more information see my comment at: #12006 (comment)

@ficristo
Copy link
Collaborator

I find a bit weak to rely on the semi-private API and the discussion linked above seems quite old.
I'm not against per se but is this the only way?

@petetnt
Copy link
Collaborator Author

petetnt commented Jan 29, 2016

@ficristo It's the only way if you want to use the same configuration over different domains (for example extensions, web workers and node domains): it's a limitation of RequireJS. The semi-private API part I agree with, but then again it hasn't changed in over 5 years: if it does, there will be a similar public API available I am sure. If not, you can always just reduplicate the configuration.

It isn't a very meaningful change as of now: everything will work as they used to. It's a blocker for something similar to #12006 (my npm concept) though and overall a low hanging fruit that falls under "nice to have".

@ficristo
Copy link
Collaborator

Thank you for the clarification.

// Convert the relative paths to absolute
Object.keys(globalPaths).forEach(function (key) {
globalPaths[key] = PathUtils.makePathAbsolute(srcPath + "/" + globalPaths[key]);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I hope not, but if we'll find problems with this approach we'll have to dupe again this configuration:

paths: {

right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ficristo, yup. There shouldn't be any problems with the approach however.

@petetnt
Copy link
Collaborator Author

petetnt commented Aug 20, 2016

Updated this PR against the latest master (non-global PathUtils), prefixed the API and made a note about internal use only.

@@ -41,7 +41,8 @@ define(function (require, exports, module) {
FileUtils = require("file/FileUtils"),
Async = require("utils/Async"),
ExtensionUtils = require("utils/ExtensionUtils"),
UrlParams = require("utils/UrlParams").UrlParams;
UrlParams = require("utils/UrlParams").UrlParams,
PathUtils = require("thirdparty/path-utils/path-utils");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small indentation issue here.

@ficristo
Copy link
Collaborator

Do you have tryed it with an installer?

@petetnt
Copy link
Collaborator Author

petetnt commented Aug 20, 2016

Fixed the indentation issue and added a missing piece from the original fix that had gone missing after rebasing.

Haven't tried it with installer, but extensions seem to load and install just fine.

@ficristo
Copy link
Collaborator

I tryed with a custom build installer on Linux (Ubuntu) and the TODO extension worked.
It works on Windows 10 too (I didn't try the installer here).
LGTM, thank you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants