Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update rollup to version 2.50.5 #8341

Merged
merged 5 commits into from
Jun 2, 2021
Merged

Update rollup to version 2.50.5 #8341

merged 5 commits into from
Jun 2, 2021

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jun 1, 2021

It appears our closing of this old @renovate PR to update Rollup to v2 caused @renovate to stop opening PRs to update Rollup to any v2.x.y version, so our version of Rollup has been out of date since then.

Though obviously not ideal, this major version lag hasn't been a problem for us because Rollup v1 still works very well, and we use it only to generate CommonJS bundles, which is a relatively stable build target (CommonJS hasn't changed much lately). We rely on other tools like tsc for critical stuff like TypeScript compilation, and we have numerous ways to verify the output of our build system. With that said, Rollup v2 has some great new features that I'm looking forward to trying, such as output.inlineDynamicImports!

To verify these changes, I compared the output of the build system before and after this upgrade (taking f76ec87 and 2ae6ddd as given/prior):

diff --git a/@apollo/client/apollo-client.cjs.js b/@apollo/client/apollo-client.cjs.js
index 65e3dc0..16a05c0 100644
--- a/@apollo/client/apollo-client.cjs.js
+++ b/@apollo/client/apollo-client.cjs.js
@@ -4,13 +4,13 @@ Object.defineProperty(exports, '__esModule', { value: true });
 
 var tslib = require('tslib');
 var tsInvariant = require('ts-invariant');
-var graphql = require('graphql');
+var equality = require('@wry/equality');
 var zenObservableTs = require('zen-observable-ts');
 require('symbol-observable');
-var equality = require('@wry/equality');
+var graphql = require('graphql');
 var optimism = require('optimism');
-var trie = require('@wry/trie');
 var context = require('@wry/context');
+var trie = require('@wry/trie');
 var graphqlTag = require('graphql-tag');
 
 function shouldInclude(_a, variables) {
@@ -681,7 +681,7 @@ function isNonNullObject(obj) {
     return obj !== null && typeof obj === 'object';
 }
 
-var hasOwnProperty = Object.prototype.hasOwnProperty;
+var hasOwnProperty$2 = Object.prototype.hasOwnProperty;
 function mergeDeep() {
     var sources = [];
     for (var _i = 0; _i < arguments.length; _i++) {
@@ -718,7 +718,7 @@ var DeepMerger = (function () {
         }
         if (isNonNullObject(source) && isNonNullObject(target)) {
             Object.keys(source).forEach(function (sourceKey) {
-                if (hasOwnProperty.call(target, sourceKey)) {
+                if (hasOwnProperty$2.call(target, sourceKey)) {
                     var targetValue = target[sourceKey];
                     if (source[sourceKey] !== targetValue) {
                         var result = _this.reconciler.apply(_this, tslib.__spreadArrays([target, source, sourceKey], context));
@@ -1588,6 +1588,7 @@ var ApolloCache = (function () {
     return ApolloCache;
 }());
 
+exports.Cache = void 0;
 (function (Cache) {
 })(exports.Cache || (exports.Cache = {}));
 
@@ -3548,6 +3549,7 @@ var ApolloError = (function (_super) {
     return ApolloError;
 }(Error));
 
+exports.NetworkStatus = void 0;
 (function (NetworkStatus) {
     NetworkStatus[NetworkStatus["loading"] = 1] = "loading";
     NetworkStatus[NetworkStatus["setVariables"] = 2] = "setVariables";
@@ -4519,7 +4521,7 @@ function shouldWriteResult(result, errorPolicy) {
     return writeWithErrors;
 }
 
-var hasOwnProperty$2 = Object.prototype.hasOwnProperty;
+var hasOwnProperty = Object.prototype.hasOwnProperty;
 var QueryManager = (function () {
     function QueryManager(_a) {
         var cache = _a.cache, link = _a.link, _b = _a.queryDeduplication, queryDeduplication = _b === void 0 ? false : _b, onBroadcast = _a.onBroadcast, _c = _a.ssrMode, ssrMode = _c === void 0 ? false : _c, _d = _a.clientAwareness, clientAwareness = _d === void 0 ? {} : _d, localState = _a.localState, assumeImmutableResults = _a.assumeImmutableResults;
@@ -4670,7 +4672,7 @@ var QueryManager = (function () {
                 this.queries.forEach(function (_a, queryId) {
                     var observableQuery = _a.observableQuery;
                     var queryName = observableQuery && observableQuery.queryName;
-                    if (!queryName || !hasOwnProperty$2.call(updateQueries_1, queryName)) {
+                    if (!queryName || !hasOwnProperty.call(updateQueries_1, queryName)) {
                         return;
                     }
                     var updater = updateQueries_1[queryName];

All of these changes seem harmless/fine/good to me!

This should prevent Rollup from generating unnecessary code like

  var invariant__default = /*#__PURE__*/_interopDefaultLegacy(invariant);
It appears our closing of this old @renovate PR to update Rollup to v2
caused @renovate to stop opening PRs to update Rollup to any v2.x.y
version, so our version of Rollup has been out of date since then:
#6033

Though obviously not ideal, this major version lag hasn't been a problem
for us because Rollup v1 still works very well, and we use it only to
generate CommonJS bundles, which is a relatively stable build target
(CommonJS hasn't changed much lately). We rely on other tools like tsc
for critical stuff like TypeScript compilation, and we have numerous
ways to verify the output of our build system.

With that said, Rollup v2 has some great new features that I'm looking
forward to trying, such as `output.inlineDynamicImports`.
Although the upgrade from rollup@1.31.1 to rollup@2.x has been almost
entirely seamless (yay!), rollup@2.26.8 included a change
(rollup/rollup#3753) that made it possible for
the options.external function to receive fully resolved, absolute ID
strings, so our implementation of that function needed to be updated to
accommodate that possibility.
This affects only `react`, `prop-types`, and `hoist-non-react-statics`
packages, which all export an object that can be considered a module
namespace object, so this transform is not needed:

  function _interopNamespace(e) {
      if (e && e.__esModule) return e;
      var n = Object.create(null);
      if (e) {
          Object.keys(e).forEach(function (k) {
              n[k] = e[k];
          });
      }
      n['default'] = e;
      return Object.freeze(n);
  }

  var React__namespace = /*#__PURE__*/_interopNamespace(React);
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching and fixing this @benjamn! (and sorry for inadvertently causing renovate to skip all of 2.x 😳)

@benjamn benjamn merged commit 8c80f1c into release-3.4 Jun 2, 2021
@benjamn benjamn deleted the update-rollup-to-v2 branch June 2, 2021 13:10
benjamn added a commit that referenced this pull request Jun 25, 2021
Addressing concerns raised by @dylanwulf on the PR that introduced this
code: #8341 (review)
@hwillson hwillson removed this from the MM-2021-06 milestone Jul 29, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants