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

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

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.

Comment 1

[Tom Tromey :tromey]

Attachment: Bug 1310702 - use webpack-like require.context in devtools l10n;

Review commit: https://reviewboard.mozilla.org/r/87234/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87234/

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 3

[Tom Tromey :tromey]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87234/diff/1-2/

Comment 4

[Tom Tromey :tromey]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87234/diff/2-3/

Comment 5

[Tom Tromey :tromey]

This version removes the last "chrome://" prefixes from uses of LocalizationHelper.

Comment 6

[Tom Tromey :tromey]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87234/diff/3-4/

Comment 7

[Tom Tromey :tromey]

... and fix eslint.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad425b5dd87b
use webpack-like require.context in devtools l10n; r=jryans

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ad425b5dd87b