add webpack-like require.context to addon-sdk require

RESOLVED FIXED in Firefox 52

Status

enhancement
P1
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

unspecified
Firefox 52
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [devtools-html])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
This comes from bug 1291049.
There, we are using webpack for the inspector-in-content project.
However, devtools l10n.js uses dynamic requires which webpack doesn't like.
The solution we found to this is to introduce require.context and have l10n.js
use different require contexts for webpack.
Whiteboard: [devtools-html] → [devtools-html] [triage]
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
(Assignee)

Updated

3 years ago
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Iteration: --- → 52.3 - Nov 7
Priority: P2 → P1

Comment 2

3 years ago
mozreview-review
Comment on attachment 8802996 [details]
Bug 1310702 - use webpack-like require.context in devtools l10n;

https://reviewboard.mozilla.org/r/87234/#review86644

It's a little strange, but it makes sense overall.  Thanks for working on it!

::: addon-sdk/source/lib/toolkit/loader.js:800
(Diff revision 1)
>      return uri;
>    }
>  
> +  // This is like webpack's require.context.  It returns a new require
> +  // function that prepends the prefix to any requests.
> +  require.context = (prefix) => {

Nit: can drop parens for single arg

::: devtools/shared/l10n.js:11
(Diff revision 1)
>  const parsePropertiesFile = require("devtools/shared/node-properties/node-properties");
>  const { sprintf } = require("devtools/shared/sprintfjs/sprintf");
>  
>  const propertiesMap = {};
>  
> +// Some shenanigans are needed for LocalizationHelper's dynamic

Please explain in comments _why_ `require.context` is useful for Webpack with dynamic requires (like you did on IRC).  When reading the code, it seems like a no op (which it is for the DevTools loader).

::: devtools/shared/l10n.js:15
(Diff revision 1)
> +const reqShared = require.context("raw!devtools-shared/locale/",
> +                                  true, /^.*\.properties$/);
> +const reqClient = require.context("raw!devtools/locale/",
> +                                  true, /^.*\.properties$/);
> +const reqGlobal = require.context("raw!global/locale/",
> +                                  true, /^.*\.properties$/);

I don't particularly like how these module IDs for these files allows the chrome:// paths to bleed through.  I think a better design would be to use real path entries in the loader instead.  I filed bug 1312041 about this.

Anyway, that doesn't really affect the logic of what's happening here, it may just change the module IDs later.
Attachment #8802996 - Flags: review?(jryans) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

3 years ago
This version removes the last "chrome://" prefixes from uses of LocalizationHelper.
Comment hidden (mozreview-request)
(Assignee)

Comment 7

3 years ago
... and fix eslint.

Comment 8

3 years ago
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad425b5dd87b
use webpack-like require.context in devtools l10n; r=jryans

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ad425b5dd87b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.