Allow to use require("react") and others in browser-loader

[juliandescottes]

In Bug 1450071, I am trying to use fluent (new l10n API from localization team) to localize the application panel. fluent-react is a library that provides react bindings to use fluent. It works almost as is when imported in DevTools, except that we have to udpate the require() to point to the vendored versions of react/react-prop-types/etc...:

require('react') -> require('devtools/client/shared/vendor/react')

Could we consider mapping 'react' to 'devtools/client/shared/vendor/react' directly in our browser-loader to facilitate reusing such libraries? It should also simplify the upgrading of both react and react-redux.

[julienw]

I don't see downsides.

[MikeRatcliffe]

Yup, seems right to me... especially considering that there are multiple versions of React in the tree used by different teams.

Maybe we should consider a folder available to all teams? Then people could just import the version they want.

[ochameau]

I imagine you are skeptical about this because of the scary comments we put there: https://searchfox.org/mozilla-central/source/devtools/shared/Loader.jsm#33

We suggested to stop adding new such "magic" mapping for many reasons, that changed over time:

• each tool was starting to introduce its own very specific mapping It was starting to be hard to guess where a given module was localed in the source tree as it could map to very random location within devtools folder. gcli is a good example of this. It maps to something hidden in a deep sub folder of devtools.
• it allow to decouple require paths from package layout acorn/util/walk is a good exampe of this. The require path doesn't match the path used in firefox package (omni.jar). This practice forces to have a build step and introduce unecessary difference between source and runtime layout.
• at some point Jetpack/Addon-SDK was trying to be node-compatible, so that we can use any modules from npm (via package.json) and these magic mapping would easily collide with standard node modules as well as npm-imported ones.

A "magic" mapping is anything that isn't an absolute patch from mozilla-central root folder.

Now, having said that, it sounds fine to do that just for "react". Especially given its broad usage. But we should think it through if this a ramp to do the same for redux and other vendors. In such case we should make it clear where are these magic mapping mapped to. We shouldn't expect everyone to know about each individual mappings hidden in Loader.jsm.

[juliandescottes]

Yes, I asked because of the scary comments :) For fluent-react, I need react and react-prop-types, so I would like to propose adding these two to the list. In theory I would need "fluent" as well but I hope we can have a custom build script for fluent-react that will bundle the fluent deps inside of fluent-react.

The related question is should we keep using require("devtools/client/shared/vendor/react") in devtools code or use the shorter (but obscure) require("react"). I also don't like magic mappings and my main motivation here is to avoid rewriting requires when importing external libraries. I am ok with using the new react mappings only for vendored code.

[juliandescottes]

After discussion with @ochameau and @jryans we are refining the proposal. It should make sense to expose all our vendored libraries as "top-level" libraries in require. So rather than picking a select few, all the vendored libraries should be added to the mappings available in Loader.jsm.

See the current contents of our vendor folder: https://searchfox.org/mozilla-central/source/devtools/client/shared/vendor

Proposal is to:

• cleanup client/shared/vendor if needed
• add mappings for each library in Loader.jsm

@jryans you mentioned using linting to avoid confusing usage of the mappings. Was your idea to prevent adding new mappings that would not be in devtools/client/shared/vendor or was it something else?

[jryans]

@jryans you mentioned using linting to avoid confusing usage of the mappings. Was your idea to prevent adding new mappings that would not be in devtools/client/shared/vendor or was it something else?

I am not sure if the linting is the right approach (maybe, maybe not). If we are manually adding loader mappings for each vendored package, then perhaps some kind of assert or mochitest for the loader that checks that we're only mapping these "approved" directories?

The main goal is to avoid returning to the "free for all" mapping scenario where random files were being mapped for seemingly no good reason. Of course, people can always edit assertion and tests in their patches, so it's not obvious to me what the right level of checks are here vs. documenting the right thing and expecting people to follow it.

[juliandescottes]

This RFC was accepted, we should now create a bug to:

• cleanup vendor folder
• add mapping for each library
• add some logic to prevent new mappings from being added

[juliandescottes]

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