inspect-js / node-deep-equal

node's assert.deepEqual algorithm
MIT License
778 stars 109 forks source link

Upgrading to 2.0.1 results in 'return' outside of function error #78

Closed n8sabes closed 4 years ago

n8sabes commented 4 years ago

It seems that upgrading to node-deep-equal 2.0.1 results in an error. I'm seeing other posts on this issue within the last few days against other projects, so it's unclear the true origin.

./node_modules/is-map/index.js
SyntaxError .../node_modules/is-map/index.js: 'return' outside of function (12:1)

  10 |      return false;
  11 |  };
> 12 |  return;
     |  ^
  13 | }
  14 |
  15 | var $mapHas = $Map ? Map.prototype.has : null;

DOWNGRADING to deep-equal 1.1.1 resolves the problem in my React (CRA) project.

ljharb commented 4 years ago

This isn’t an error; top level returns are perfectly valid in a CJS module. Have you customized your CRA config at all?

n8sabes commented 4 years ago

Hi @ljharb, I recall the project was a clean CRA --typescript base, but the project has evolved into a more mature project over the last month.

I upgrade packages once a week during development cycles with yarn upgrade --latest, which I did tonight, when the error manifested. Upon rolling back packages one at a time, I found rolling back deep-equal to 1.1.1 resolve the build error.

ljharb commented 4 years ago

Indeed; v2 is the one that uses is-map/is-set, which have a top-level return.

The issue is that something that's interacting with node-style CJS modules isn't supporting a decade-old feature of those modules. I'd be surprised if it was Webpack.

ljharb commented 4 years ago

eg, https://github.com/webpack/webpack/issues/8509 is resolved, https://github.com/webpack/webpack/pull/8510 is merged, released in webpack v4.31+. What version of webpack are you using?

n8sabes commented 4 years ago

@ljharb, I'm not including webpack myself, but executing npm list webpack reveals the following:

└─┬ react-scripts@3.3.0
  └── webpack@4.41.2
n8sabes commented 4 years ago

Here is a simple set of steps to create a clean project that exhibits the anomaly:

yarn create react-app my-app --template typescript
cd my-app
yarn add deep-equal
vi src/index.tsx

# Add lines to index.tsx before render()
    import deepEqual from "deep-equal"; // NEW LINE
    deepEqual({}, {});                  // NEW LINE

yarn build

Output:

Failed to compile.

./node_modules/is-map/index.js
SyntaxError: .../my-app/node_modules/is-map/index.js: 'return' outside of function (12:1)

  10 |      return false;
  11 |  };
> 12 |  return;
     |  ^
  13 | }
  14 |
  15 | var $mapHas = $Map ? Map.prototype.has : null;

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
mahnunchik commented 4 years ago

The same issue...

ljharb commented 4 years ago

Looks like babel needs to have the allowTopLevelReturn option enabled if you're going to be transpiling CJS node_modules.

n8sabes commented 4 years ago

@ljharb, deep-equal is still not working with a vanilla CRA project.

I just recreated a clean my-app with deep-equal as outlined above, but get the same error from is-weakmap:

./node_modules/is-weakmap/index.js
SyntaxError: .../my-app/node_modules/is-weakmap/index.js: 'return' outside of function (12:1)

  10 |      return false;
  11 |  };
> 12 |  return;
     |  ^
  13 | }
  14 |
  15 | var $mapHas = $WeakMap ? $WeakMap.prototype.has : null;

Are you saying you have to somehow modify a CRA install with allowTopLevelReturn to use deep-equal? If so, how do you do that, or am I misunderstanding?

n8sabes commented 4 years ago

Can we re-open this issue until there is a solution to use deep-equal again with CRA generated React apps?

ljharb commented 4 years ago

We could have, but you opened up #79 before i had a chance to.

n8sabes commented 4 years ago

Sorry about that -- Thought we lost you already on this issue thread. Thank you for your efforts on this! Sincerely appreciated.

Moving over to #79.