Use package.json names for AMD bundles

[appsforartists]

Currently, the AMD distributions don't specify module names. Therefore, when they are inlined in a bundle or imported with a script tag:

<script src = "https://unpkg.com/requirejs@2.3.2/require.js"></script>
<script src = "https://unpkg.com/react@16.2.0/umd/react.production.min.js"></script>
<script src = "https://unpkg.com/react-dom@16.2.0/umd/react-dom.production.min.js"></script>
<script>
  define('', ['require', 'exports', 'react', 'react-dom'] // …
    var React = require('react');

RequireJS throws mismatched define errors:

Uncaught Error: Mismatched anonymous define() module: function (React) { 'use strict';
Uncaught Error: Mismatched anonymous define() module: function () { 'use strict';

This patch passes the bundle names into rollup, so the AMD definitions can use them.

By naming the builds, they can be easily consumed from the Bazel build tool. Bazel inlines AMD dependencies, but doesn't otherwise transpile their module syntaxes.

I have minimal familiarity with AMD. I don't believe adding module names should affect other environments.

[gaearon]

needs trailing comma (just run yarn prettier)

[appsforartists]

I thought I'd run that. I'll double-check.

[appsforartists]

Fixed.

[gaearon]

Is it worth still trying to test this? Does this trigger some other code path in RequireJS?

Maybe we could copy the fixtures?

[appsforartists]

I wasn't sure which route you'd prefer. I did test both before opening the PR.

Options:

1. Merge as-is.
2. Include react-dom as a script and react in this map (or vice versa - we could even have dev test one combo and prod test the other).
3. Make a new pair of fixtures that tests the map style.

Which would you prefer? If you'd like both, would they each live as folders in fixtures, or as subfolders of fixtures/requirejs?

[gaearon]

Top-level folders in fixtures sound fine, let's keep both versions

[appsforartists]

I've never used RequireJS before, so I'm certainly not the expert. Perusing its source, it looks like define(name is inserted into defQueue, but it's not obvious how config.paths gets registered.

I didn't originally add 2 sets of fixtures because ensuring that define(name and paths[name] worked the same way seemed like a RequireJS concern (rather than a React one), but I'm happy to add a second set if you think it's worthwhile. (I also don't know who/how fixtures are tested - is it just a manual smoketest, or is checking them part of a process?)

[gaearon]

Just a manual smoketest.

I'm not an expert either—do what you think is reasonable and we'll try to review :-)

[appsforartists]

I'm inclined to leave it as-is. My inclination is that it's RequireJS's responsibility to ensure that config.paths works with define(name, and that adding fixtures to React that assert that adds noise more than value.

I feel more confident in this direction because I did test the old style before updating the fixture - they both work.

[appsforartists]

Oh, I just saw that you did request both fixtures. It got lost in the thread. I'll add them.

[gaearon]

Can you please bring it up to date?

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

[stale]

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!