Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CLOSED] Use the global RequireJS config for ExtensionLoader. Fixes #1087 #10373

Open
core-ai-bot opened this issue Aug 30, 2021 · 7 comments
Open

Comments

@core-ai-bot
Copy link
Member

Issue by petetnt
Saturday Jan 02, 2016 at 12:25 GMT
Originally opened as adobe/brackets#12041


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: adobe/brackets#12006 (comment)


petetnt included the following code: https://github.com/adobe/brackets/pull/12041/commits

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Friday Jan 29, 2016 at 19:42 GMT


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?

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Friday Jan 29, 2016 at 20:18 GMT


@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".

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Saturday Jan 30, 2016 at 08:36 GMT


Thank you for the clarification.

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Saturday Aug 20, 2016 at 11:41 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Saturday Aug 20, 2016 at 11:44 GMT


Do you have tryed it with an installer?

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Saturday Aug 20, 2016 at 11:59 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Sunday Aug 21, 2016 at 08:47 GMT


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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant