Fix externalization of 'lowPriorityWarning' in fb builds

[flarnie]

Thanks to @spicyj for pairing with me on this.

what is the change?:

• Add two more special cases for 'lowPriorityWarning' in 'modules.js',
  treating it the same as 'ReactCurrentOwner'.

why make this change?:
Without this, the build was including 'lowPriorityWarning' seemingly both as an external module and as part of the bundle in fb builds.

test plan:
Ran yarn build and inspected the React-dev build. lowPriorityWarning did not get bundled in this time.

issue:
None yet - @gaearon flagged this for me directly.

[trueadm]

LGTM

[gaearon]

Is there anything we can change in build system so that it wouldn't compile in the first place instead of producing weird output? @trueadm

[trueadm]

@gaearon it's a tricky one though – as this should build, but only for non-FB? Unless I misunderstand you.

[gaearon]

I mean that instead of producing broken output for FB bundles, it would be nice if it failed to build.
By broken output I mean

var invariant = require('fbjs/lib/invariant')
var warning = require('fbjs/lib/warning')
require('lowPriorityWarning'); // it never makes sense to have top level require like this

// ...

function lowPriorityWarning() {
  // ...
}

[trueadm]

@gaearon so lowPriorityWarning was being required at the top and the module was also being bundled in the code?

I am not sure why this is. It sounds like a bug in Rollup maybe. I'll CC @Rich-Harris, if we can get a test case, maybe he can explain why this is the case.

[gaearon]

@trueadm

It is a bit hard to explain—you can revert this commit locally, run npm run build -- core --type=FB, and see the issue.

Our intention was to make lowPriorityWarning an external in FB builds so that they just require() it rather than try to bundle the local version. @flarnie tried to accomplish it what seemed to be an intuitive way, by adding it to externalModules array: https://github.com/facebook/react/pull/9650/files#diff-1473b7084b44dd8e587bd31a6d667d9aR165.

However this produced a build that both had (an unused) require('lowPriorityWarning') at the top and a bundled version of it (that was used by the code). This PR was required to remove the bundled version and actually get the external version to be used.

This is not the first time we make this mistake (whatever it is). I remember having the same issue a few times before when I tried to externalize a module. I expect that adding it to externalModules will make it an external module, but it seems like this is not enough, and it’s also necessary to delete it from some other places (as shown in this PR).

I am suggesting that we either make externalModules behave as we’d intuitively expect (they make things external), or we at least fail the build instead of producing a build with unused top-level require.

[trueadm]

@gaearon I understand, thank you for explaining that. I think the confusion is by the naming and the misconception that this works like Webpack – which it does not. I'll make it work like Webpack externals next week. It shouldn't take too much time to make, I just need to re-test everything extensively :)

[gaearon]

Awesome, thanks! I don't think it's necessary to make it work specifically like Webpack, but it shouldn't be so easy to end up with a broken state. Maybe we could just rename externalModules to something less ambigious and make it more discoverable how to add internal FB "externals" (whatever we call them).

[gaearon]

If the correct mental model for "external" modules today is that these are the modules that are allowed to be required even if they don't exist, it is still confusing that they're required when they do exist but also bundled. That empty unused injected top-level require is the concerning part for me. This is never what we want.

[trueadm]

@gaearon I agree, although this is definitely a harder one to solve as it's how Rollup works by default.

I do however have a solution without changing Rollup: we define externals for DEV and non DEV bundles, like we do now with externalModule, but we have two of them instead of one. Then we make externals that are not used throw and error :)

To be honest, I'm not sure why I didn't think of this before. This is by far the best solution in my eyes, I guess I was trying to hard to automate this – but having them separate is probably more declarative in descriptive anyway.