961194 - Pull out utils from toolkit/loader for organization

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P3)

Description

[Jordan Santell [:jsantell] [@jsantell]]

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.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 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).

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8368355 [details] [review]
GH PR 1369

Looks good to me

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

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

[[github robot]]

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

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

This broke the fx-team tree tests, see bug 972925

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

I had to revert the patch here https://github.com/mozilla/addon-sdk/commit/036238d8bd1bfbbbc2a44467be646711e3aeed19

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Noted, was looking into this and a bit stumped. Will try to fix this up for a future release

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Unassigning myself from bugs I haven't gotten around to

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

I wanted to look at this but I won't be able to.

Comment 12

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562