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

ReferenceError: window is not defined in 0.32.0 #339

Closed
quisido opened this issue Jun 3, 2022 · 16 comments · Fixed by #340
Closed

ReferenceError: window is not defined in 0.32.0 #339

quisido opened this issue Jun 3, 2022 · 16 comments · Fixed by #340
Assignees
Labels
kind: regression Specific type of bug -- past behavior that worked is now broken problem: removed issue template OP removed the issue template without good cause scope: dependencies Issues or PRs about updating a dependency scope: upstream Issue in upstream dependency solution: workaround available There is a workaround available for this issue

Comments

@quisido
Copy link

quisido commented Jun 3, 2022

What happens and why it is wrong

Environment

  • Yarn 3
Versions

The command to use envinfo throws an error, so I'm resorting to copy-paste. All versions are latest.

    "rollup": "^2.75.5",
    "rollup-plugin-typescript2": "^0.32.0",
    "typescript": "^4.7.2"

The error resides here:

if (typeof commonjsRequire === "function") {
  try {
    lodash = {
      clone: require("lodash/clone"),
      constant: require("lodash/constant"),
      each: require("lodash/each"),
      filter: require("lodash/filter"),
      has:  require("lodash/has"),
      isArray: require("lodash/isArray"),
      isEmpty: require("lodash/isEmpty"),
      isFunction: require("lodash/isFunction"),
      isUndefined: require("lodash/isUndefined"),
      keys: require("lodash/keys"),
      map: require("lodash/map"),
      reduce: require("lodash/reduce"),
      size: require("lodash/size"),
      transform: require("lodash/transform"),
      union: require("lodash/union"),
      values: require("lodash/values")
    };
  } catch (e) {
    // continue regardless of error
  }
}

if (!lodash) {
  lodash = window._;
}

The line saying to use window._ throws an error because window does not exist in the Node environment.

This is newly introduced in 0.32.0. Reverting to 0.31.0 fixes this.

@agilgur5 agilgur5 added the problem: removed issue template OP removed the issue template without good cause label Jun 3, 2022
@agilgur5 agilgur5 changed the title ReferenceError: window is not defined ReferenceError: window is not defined in 0.32.0 Jun 3, 2022
@agilgur5
Copy link
Collaborator

agilgur5 commented Jun 3, 2022

The error resides here:

This code exists in 0.31.0 as well, can see the dist diff (it's in both + and - as the code merely moved around).
The code is also from lodash internals, and not rpt2 source code.

The line saying to use window._ throws an error because window does not exist in the Node environment.

rpt2 builds itself with itself, which is done with Rollup and Node, so I am unable to reproduce this in a Node environment.

This is newly introduced in 0.32.0. Reverting to 0.31.0 fixes this.

The only lodash related PR in 0.32.0 is #328 , which basically just reduces the usage of lodash. It's still used in a few places though and still part of the bundle as such. It's the same version though and it is pinned to a specific version too (not that lodash gets many updates nowadays).

I can't really pinpoint what would cause any change here, and you didn't provide a reproduction or a stacktrace / verbosity: 3 log per the issue template, so can't really investigate further either.

@agilgur5 agilgur5 added the solution: can't repro An attempt to reproduce has been tried and failed label Jun 3, 2022
@quisido
Copy link
Author

quisido commented Jun 3, 2022

I unfortunately didn't snapshot it at a point in time, because I'm trying to do a larger effort around dependency upgrades. This is in a private repository anyway. :(

Here's the stack trace:

PS ...\REPO> yarn rollup   
[!] ReferenceError: window is not defined
ReferenceError: window is not defined
    at Object.<anonymous> (...\REPO\.yarn\__virtual__\rollup-plugin-typescript2-virtual-387384c206\0\cache\rollup-plugin-typescript2-npm-0.32.0-9a7f8c5b22-481b4288e8.zip\node_modules\rollup-plugin-typescript2\dist\rollup-plugin-typescript2.cjs.js:20377:3)
    at Module._compile (node:internal/modules/cjs/loader:1103:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1155:10)
    at require$$0.Module._extensions..js (...\REPO\.pnp.cjs:21415:33)
    at Object.require.extensions.<computed> [as .js] (...\REPO\.yarn\cache\rollup-npm-2.75.5-7f19e4b2e6-fa3e61959e.zip\node_modules\rollup\dist\shared\loadConfigFile.js:617:17)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.require$$0.Module._load (...\REPO\.pnp.cjs:21255:14)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (...\REPO\.yarn\__virtual__\@monorepo-template-rollup-config-virtual-198f84a7ca\0\cache\@monorepo-template-rollup-config-npm-2.1.0-f4bb5fa311-4072763686.zip\node_modules\@monorepo-template\rollup-config\src\utils\rollup-config.cjs:1:21)

The @monorepo-template/rollup-config dependency that calls typescript2 can be found here.

When I finish the upgrade overall, I'll take rollup-plugin-typescript2 off ~0.31.0 and put it back on ^0.32.0 and see if it still occurs.

It may be related to the workspace package having "type": "module" in its package.json, as I notice the dependency is .cjs. I didn't experience this in another repo of mine that is "type": "commonjs".

@agilgur5
Copy link
Collaborator

agilgur5 commented Jun 3, 2022

Here's the stack trace:

Thanks for providing this. It looks like you're using Yarn PnP too, which is possibly related too?
We also normally ask to run with clean: true as well, but in this case, I think that wouldn't make a difference as this happens during parsing of rpt2's dist code.

It may be related to the workspace package having "type": "module" in its package.json, as I notice the dependency is .cjs. I didn't experience this in another repo of mine that is "type": "commonjs".

Hmmm... possibly. That try/catch of lodash's gets hit when there's an error from require.
Did you have "type": "module" with 0.31.0 as well? A regression would suggest that there's a race condition somewhere, as the only thing that changed was the order in which lodash gets imported and how frequently it gets imported.

So rpt2 does export an ESM version in its package.json#module field etc, but that old convention was never quite supported everywhere (lots of bundlers like Webpack did support it though, but not Node itself).
You can try importing that directly instead? import rpt2 from "rollup-plugin-typescript2/dist/rollup-plugin-typescript2.es.js"

Related, but that's a good reminder that we should add the newer package.json#exports in, especially now that the spec has stabilized and finally seems to be properly forward and backward-compatible (c.f. #286 where tslib's exports broke in Node 17).

When I finish the upgrade overall, I'll take rollup-plugin-typescript2 off ~0.31.0 and put it back on ^0.32.0 and see if it still occurs.

Sounds good. A minimal repro would help quite a bit if you do hit it then. My suspicion is that it's something in your environment causing this, although I have no idea why it's different between 0.31.0 and 0.32.0 since it's coming from within the same version of the same bundled lodash code.

@shevernitskiy
Copy link

shevernitskiy commented Jun 3, 2022

I've got the same issue after upgrading from 0.31.2 to 0.32.0.
Yarn 2 - v1.23.0-20220130.1630

package.json
{
  "name": "gismeteo",
  "version": "1.0.6",
  "main": "lib/gismeteo.js",
  "typings": "lib/gismeteo.d.ts",
  "module": "lib/gismeteo.esm.js",
  "files": [
    "lib",
    "package.json",
    "README.md",
    "LICENSE"
  ],
  "license": "MIT",
  "author": "sheverniskiy",
  "description": "Gismeteo weather (unofficial)",
  "keywords": [
    "gismeteo",
    "weather",
    "parser",
    "scraper"
  ],
  "repository": {
    "type": "git",
    "url": "git+https://github.com/shevernitskiy/gismeteo.git"
  },
  "scripts": {
    "start": "rollup -c -w",
    "build": "rollup -c",
    "build:watch": "rollup -c -w",
    "prepare": "rollup -c",
    "lint": "eslint . --ext .ts",
    "test": "jest gismeteo.test.ts",
    "test:coverage": "jest --coverage gismeteo.test.ts",
    "playground": "jest playground.test.ts",
    "release": "np patch"
  },
  "dependencies": {
    "axios": "^0.27.2",
    "cheerio": "^1.0.0-rc.11",
    "is-number": "^7.0.0",
    "moment": "^2.29.3",
    "tslib": "~2.4.0",
    "user-agents": "^1.0.1027"
  },
  "devDependencies": {
    "@types/is-number": "^7.0.3",
    "@types/jest": "~27.4",
    "@types/node": "^17.0.35",
    "@types/user-agents": "^1.0.2",
    "@typescript-eslint/eslint-plugin": "~5.27",
    "@typescript-eslint/parser": "~5.14",
    "eslint": "~8.16",
    "eslint-config-prettier": "~8.5",
    "eslint-plugin-jest": "~26.1",
    "jest": "^28.1.0",
    "rollup": "^2.75.0",
    "rollup-plugin-typescript2": "^0.32.0",
    "ts-jest": "^28.0.3",
    "typescript": "^4.7.2"
  }
}

@agilgur5
Copy link
Collaborator

agilgur5 commented Jun 3, 2022

@shevernitskiy I see your library is open-source: https://github.com/shevernitskiy/gismeteo.
It's not a minimal repro, but a repro still helps a lot! I'll look into this tomorrow.

I also notice that similar to @CharlesStover , both of you were updating several dependencies at once.
And both using Yarn.

I've upgraded to rpt2 0.32.0 in some of my other libraries and haven't encountered this as of yet, but they all use NPM fwiw.

@agilgur5
Copy link
Collaborator

agilgur5 commented Jun 3, 2022

Found your CI log as well that reproduces it.
Interestingly enough, that actually points to a file within graphlib, a different dependency that rpt2 uses for it's cache implementation (graph as in dependency graph) 🤔 That's also pinned though and hasn't been updated in ~2 years

Hmm...

@agilgur5 agilgur5 added the kind: regression Specific type of bug -- past behavior that worked is now broken label Jun 3, 2022
@agilgur5
Copy link
Collaborator

agilgur5 commented Jun 3, 2022

Might be due to the internal upgrade of @rollup/plugin-commonjs in 5a3e58b . It's used to bundle rpt2 itself, i.e. it's own dependencies (see also #80)

@agilgur5
Copy link
Collaborator

agilgur5 commented Jun 3, 2022

Ok was able to repro the issue with both NPM and Yarn in gismeteo. Oddly enough though, I still can't repro it here or in my other libraries.

What I also found was that @rollup/plugin-commonjs was actually updated twice in this release, in the commit I mentioned above, and in 08d2f5b.

That commit moves up 3 majors and in particular this issue comment stood out to me as very relevant: rollup/plugins#1005 (comment) . That should be fixed by rollup/plugins#1038 in v22.0.0, so I'll try updating that and seeing if it fixes this issue in gismeteo

@agilgur5
Copy link
Collaborator

agilgur5 commented Jun 3, 2022

Annnd yep, this exact piece of code that requires lodash changes in that exact commit: 08d2f5b#diff-80441564774ccf83c60a1616524830d5c15a9bc194b62e2c3d9f43de805c62d0L22435

Updating to @rollup/plugin-commonjs to v22.0.0 once again changes that piece of code and seems to fix the issue when I test with either Yarn or NPM in gismeteo

Can install via my issue-339 branch to workaround this issue for now. Will make a PR shortly, but have to wait on @ezolenko for a release.

In the meantime, @CharlesStover can you confirm that installing my issue-339 branch fixes this for you as well?

yarn add --dev git+https://github.com/agilgur5/rollup-plugin-typescript2#issue-339

@agilgur5 agilgur5 added solution: workaround available There is a workaround available for this issue scope: dependencies Issues or PRs about updating a dependency scope: upstream Issue in upstream dependency and removed solution: can't repro An attempt to reproduce has been tried and failed labels Jun 3, 2022
@quisido

This comment was marked as off-topic.

@agilgur5

This comment was marked as resolved.

@quisido

This comment was marked as resolved.

@agilgur5

This comment was marked as resolved.

@mistyharsh

This comment was marked as duplicate.

@agilgur5
Copy link
Collaborator

agilgur5 commented Jun 6, 2022

@mistyharsh yes, I've already looked into this, linked diffs of the bundle, found that it's actually not in rpt2 or lodash code, but in graphlib code (which is used in rpt2's cache functionality), found the root cause to be an erroneous breaking change in @rollup/plugin-commonjs, and made a PR updating it.

That's all in the above comments made a few days ago.

You can install via my branch mentioned in the above comments to workaround this while the PR is awaiting merge and release (I have merge permissions on this repo, but not release permissions on NPM).

Also the bundled dependencies are a known thing, as I've linked in the above comments already, please see #80 for that. Upvotes and comments there may persuade ezolenko (this issue in and of itself may as well).

@agilgur5
Copy link
Collaborator

agilgur5 commented Jun 7, 2022

A bugfix has been released in 0.32.1 🎉

I've also confirmed that 0.32.1 works inside of rpt2, in one of my own libraries, and the library that I was able to repro the bug in above, gismeteo.

Thanks to everyone for submitting this issue, providing repros, etc not too long after 0.32.0's release so it could get addressed quickly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: regression Specific type of bug -- past behavior that worked is now broken problem: removed issue template OP removed the issue template without good cause scope: dependencies Issues or PRs about updating a dependency scope: upstream Issue in upstream dependency solution: workaround available There is a workaround available for this issue
Projects
None yet
4 participants