Pull out utils from toolkit/loader for organization

RESOLVED INCOMPLETE

Status

Add-on SDK
General
P3
normal
RESOLVED INCOMPLETE
4 years ago
5 months ago

People

(Reporter: jsantell, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

When using loader outside of the SDK environment, there are a few modules that use the exports of toolkit/loader within themselves.

Example: sdk/tabs includes console/traceback which uses `parseStack` and `sourceURI` from toolkit/loader.

These utilities clutter up toolkit/loader, add a larger barrier of entry to using the loader outside of the SDK (in this case, 'Components' is used in toolkit/loader and cuddlefish loader has a work around, but this reduces our pile of loader work arounds), and introduces bad smells from circular dependency and don't even really belong in a loader module.
For example, in bootstrap.js, we have to make an exception for modules including the loader itself:

`loaderOptions.modules['toolkit/loader'] = loaderSandbox.exports;`

That's extra work and scaffolding needed to use Loader with any modules using the utilities in toolkit/loader

Updated

4 years ago
Priority: -- → P3
OS: Mac OS X → All
Hardware: x86 → All
(Reporter)

Updated

4 years ago
Depends on: 964452
While bug 964452 fixes the circular dependency issue outside of the SDK environment, we should still remove some of the utilites in toolkit/loader to its own file so we don't have a 923 line file.

Should have lib/toolkit/utils.js that can be loaded as a JSM for toolkit/loader, containing loading utilities
(Reporter)

Updated

4 years ago
Summary: Pull out utils from toolkit/loader for organization and removing circular deps → Pull out utils from toolkit/loader for organization
Created attachment 8368355 [details] [review]
GH PR 1369

This pulls out loader functions into several other files, making loader logic focused on loading. All these are tested via loader's current tests, and what was previously exported is still exported to not break others using all the exports on the loader. Maybe in the future we can have individual unit tests for these functions as well.

There are still some files requiring toolkit/loader, but those are all ones that we instrument ourselves (cuddlefish, addon/runner, test/loader), so I think we'll be okay there. Just things like sdk/selection using console/traceback and ultimately toolkit/loader should be avoided.

*console-utils.js -- This is what other files (test/harness, console/traceback) are using for parsing stack traces, so it can be loaded again as a common JS module.

(The following are JSMs for ease of loading, and they use Components which is a cause of some of these recursive problems -- they should not be directly loaded via the SDK)
* parser.jsm -- This is the browserify+Reflect parsing methods we use for generating maps for native loaders
* node-resolve.jsm -- This is the `nodeResolve` method with all of its helpers. It's pretty large and contained within one method, so this is a good move i think.
* utils.jsm -- These are all other utilities used throughout toolkit/loader, and the other utils in this PR (node-resolve, parser).
Attachment #8368355 - Flags: review?(rFobic)
Attachment #8368355 - Flags: review?(evold)
Comment on attachment 8368355 [details] [review]
GH PR 1369

Looks good to me
Attachment #8368355 - Flags: review?(evold) → review+
Attachment #8368355 - Flags: review?(rFobic) → review-
Reverted changes back so only stack-utils are pulled out, and this stack-utils are what is referenced throughout the SDK where necessary, r+ by gozala with these changes

Comment 6

4 years ago
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/160be1d330cba70b13e0121f90ec078f9ac2b3a7
Bug 961194 - Pull out stack utils in loader to prevent circular dependencies in SDK

https://github.com/mozilla/addon-sdk/commit/aaf3c9061f1b8cc97deca7417937f029b9d1ea75
Merge pull request #1369 from jsantell/961194-modularize-loader-utils

fix Bug 961194 Modularize toolkit/loader, r=@erikvold, @gozala

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
This broke the fx-team tree tests, see bug 972925
Status: RESOLVED → REOPENED
Flags: needinfo?(jsantell)
Resolution: FIXED → ---
Noted, was looking into this and a bit stumped. Will try to fix this up for a future release
(Reporter)

Updated

4 years ago
Flags: needinfo?(jsantell)
Unassigning myself from bugs I haven't gotten around to
Assignee: jsantell → nobody
Flags: needinfo?(evold)
I wanted to look at this but I won't be able to.
Flags: needinfo?(evold)
https://bugzilla.mozilla.org/show_bug.cgi?id=1399562
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago5 months ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.