Node and window is not defined when building @carbon/charts #133

nstuyvesant · 2023-04-22T14:44:18Z

Use case: description, code

Similar issue to #131

[!] ReferenceError: window is not defined
ReferenceError: window is not defined
    at /Users/nates/dev/rebase/node_modules/dom-to-image-more/src/dom-to-image-more.js:50:57
    at Object.<anonymous> (/Users/nates/dev/rebase/node_modules/dom-to-image-more/src/dom-to-image-more.js:1395:3)
    at Module._compile (internal/modules/cjs/loader.js:1114:14)
    at Module._extensions..js (internal/modules/cjs/loader.js:1143:10)
    at Object.require.extensions.<computed> [as .js] (/Users/nates/dev/rebase/node_modules/rollup/dist/bin/rollup:835:17)
    at Module.load (internal/modules/cjs/loader.js:979:32)
    at Function.Module._load (internal/modules/cjs/loader.js:819:12)
    at Module.require (internal/modules/cjs/loader.js:1003:19)
    at require (internal/modules/cjs/helpers.js:107:18)
    at Object.<anonymous> (/Users/nates/dev/rebase/packages/core/rollup.config.js:14:34)

Expected behavior

Checks to see if window is undefined before referencing a property.

Actual behavior (stack traces, console logs etc)

Assumes window is defined.

Library version

3.1.4

Browsers

[x ] Chrome 112

IDisposable · 2023-04-25T23:42:29Z

What method would it be calling to getComputedStyle from (and/or atob) ? Just not crashing here doesn't really gain anything, does it?

IDisposable · 2023-04-26T01:11:31Z

Please retest using just-released v3.1.5, I'm attempting to fall back to globalThis and really can't find anything better...

nstuyvesant · 2023-04-26T20:00:04Z

Here are steps to reproduce...

Temporarily switch to Node 14 (nvm, brew, whatever you use) and make sure yarn is installed
Clone the monorepo

git clone https://github.com/nstuyvesant/carbon-charts.git
git checkout fix-pr-1540
git pull

Navigate to the core package

cd packages/core

Try to do a build of the core package

npx rollup -c

You will get an error...

[!] ReferenceError: Node is not defined
ReferenceError: Node is not defined
    at /Users/nates/dev/rebase/node_modules/dom-to-image-more/src/dom-to-image-more.js:8:26
    at Object.<anonymous> (/Users/nates/dev/rebase/node_modules/dom-to-image-more/src/dom-to-image-more.js:1392:3)
    at Module._compile (internal/modules/cjs/loader.js:1114:14)
    at Module._extensions..js (internal/modules/cjs/loader.js:1143:10)
    at Object.require.extensions.<computed> [as .js] (/Users/nates/dev/rebase/node_modules/rollup/dist/bin/rollup:835:17)
    at Module.load (internal/modules/cjs/loader.js:979:32)
    at Function.Module._load (internal/modules/cjs/loader.js:819:12)
    at Module.require (internal/modules/cjs/loader.js:1003:19)
    at require (internal/modules/cjs/helpers.js:107:18)
    at Object.<anonymous> (/Users/nates/dev/rebase/packages/core/rollup.config.js:13:34)

If you fix that error with the code I submitted for line 8...

    const ELEMENT_NODE = typeof Node === 'undefined' ? 1 : Node.ELEMENT_NODE || 1;

do a yarn build then you will get errors for line 49...

ReferenceError: window is not defined
    at /Users/nates/dev/rebase/node_modules/dom-to-image-more/src/dom-to-image-more.js:49:70
    at Object.<anonymous> (/Users/nates/dev/rebase/node_modules/dom-to-image-more/src/dom-to-image-more.js:1391:3)
    at Module._compile (internal/modules/cjs/loader.js:1114:14)
    at Module._extensions..js (internal/modules/cjs/loader.js:1143:10)
    at Object.require.extensions.<computed> [as .js] (/Users/nates/dev/rebase/node_modules/rollup/dist/bin/rollup:835:17)
    at Module.load (internal/modules/cjs/loader.js:979:32)
    at Function.Module._load (internal/modules/cjs/loader.js:819:12)
    at Module.require (internal/modules/cjs/loader.js:1003:19)
    at require (internal/modules/cjs/helpers.js:107:18)
    at Object.<anonymous> (/Users/nates/dev/rebase/packages/core/rollup.config.js:13:34)

This is line 49...

    const getComputedStyle = (global && global.getComputedStyle) || (window && window.getComputedStyle) || globalThis.getComputedStyle;

The change made for 3.1.5 replaced my PR with...

const ELEMENT_NODE = (Node && Node.ELEMENT_NODE) || 1;

but that triggers the error. I added this to the line before...

    console.log('What is the value of Node?', Node)

and I got an error...

[!] ReferenceError: Node is not defined
ReferenceError: Node is not defined
    at /Users/nates/dev/rebase/node_modules/dom-to-image-more/src/dom-to-image-more.js:8:33

I'm not sure why rollup spits out an error as I would have assumed (Node && Node.ELEMENT_NODE) would short-circuit as Node is false in this case. I only know that my two PRs solved this issue.

Thoughts?

IDisposable · 2023-04-26T20:21:03Z

I can certainly switch to the uglier syntax. It does seem like the runtime is wrong by not short-circuiting. Ugh. Will see if this can be cleaned better.

nstuyvesant · 2023-04-26T20:25:15Z

Yeah - I looked at it for a while scratching my head. I locally upgraded rollup and all plugins but it didn't help. I even switched to node 18.16 - same result.

IDisposable · 2023-04-26T20:35:54Z

It seems to be an issue with rolllup assuming everything is a module (and thus this is undefined), which breaks the entire world. This whole thing predates modules and all that stuff...it's just an ife... I reduced it down to the minimum wrapper without the whole thing and get an error when this is passed in as global and no error otherwise. See https://rollupjs.org/repl/?version=3.12.1&shareable=JTdCJTIyZXhhbXBsZSUyMiUzQW51bGwlMkMlMjJtb2R1bGVzJTIyJTNBJTVCJTdCJTIyY29kZSUyMiUzQSUyMihmdW5jdGlvbiUyMCgpJTIwJTdCJTVDbiUyMCUyMCUyMCUyMCd1c2UlMjBzdHJpY3QnJTNCJTVDbiU1Q24lMjAlMjBpZiUyMCh0eXBlb2YlMjBleHBvcnRzJTIwJTNEJTNEJTNEJTIwJ29iamVjdCclMjAlMjYlMjYlMjB0eXBlb2YlMjBtb2R1bGUlMjAlM0QlM0QlM0QlMjAnb2JqZWN0JyklMjAlN0IlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwbW9kdWxlLmV4cG9ydHMlMjAlM0QlMjBkb210b2ltYWdlJTNCJTIwJTJGJTJGJTIwZXNsaW50LWRpc2FibGUtbGluZSUyMG5vLXVuZGVmJTVDbiUyMCUyMCUyMCUyMCU3RCUyMGVsc2UlMjAlN0IlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwZ2xvYmFsLmRvbXRvaW1hZ2UlMjAlM0QlMjBkb210b2ltYWdlJTNCJTVDbiUyMCUyMCUyMCUyMCU3RCU1Q24lNUNuJTIwJTIwJTIwJTIwJTJGJTJGJTIwc3VwcG9ydCUyMG5vZGUlMjBhbmQlMjBicm93c2VycyU1Q24lMjAlMjAlMjAlMjBjb25zdCUyMGdldENvbXB1dGVkU3R5bGUlMjAlM0QlMjAoZ2xvYmFsJTIwJTI2JTI2JTIwZ2xvYmFsLmdldENvbXB1dGVkU3R5bGUpJTIwJTdDJTdDJTIwKHdpbmRvdyUyMCUyNiUyNiUyMHdpbmRvdy5nZXRDb21wdXRlZFN0eWxlKSUyMCU3QyU3QyUyMGdsb2JhbFRoaXMuZ2V0Q29tcHV0ZWRTdHlsZSUzQiU1Q24lMjAlMjAlMjAlMjBjb25zdCUyMGF0b2IlMjAlM0QlMjAoZ2xvYmFsJTIwJTI2JTI2JTIwZ2xvYmFsLmF0b2IpJTIwJTdDJTdDJTIwKHdpbmRvdyUyMCUyNiUyNiUyMHdpbmRvdy5hdG9iKSUyMCU3QyU3QyUyMGdsb2JhbFRoaXMuYXRvYiUzQiU1Q24lMjAlMjBjb25zb2xlLmxvZyhnZXRDb21wdXRlZFN0eWxlKG51bGwpKSUzQiU1Q24lMjAlMjBjb25zb2xlLmxvZyhhdG9iKCdmZicpKSUzQiU1Q24lN0QpKCklM0IlNUNuJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlMkMlMjJuYW1lJTIyJTNBJTIybWFpbi5qcyUyMiU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJvdXRwdXQlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJlcyUyMiU3RCUyQyUyMnRyZWVzaGFrZSUyMiUzQXRydWUlN0QlN0Q=

Related rollup issue rollup/rollup#4834

IDisposable · 2023-04-26T20:39:09Z

Which means that when rollup tree-shakes this, it assumes this is undefined so it KNOWS that the short-circuit doesn't work on global and thus falls back to the window even though in actual execution the this isn't undefined, and global does have a value and it runs correctly :(

Can you tweak your copy of dom-to-image-more to remove the argument in the first line (e.g. delete the global) and then in the last line, delete the this argument and see if rollup is happy AND that it runs?

IDisposable · 2023-04-26T20:41:24Z

This is what the rollupjs test code looks like:

(function () {
    'use strict';

  if (typeof exports === 'object' && typeof module === 'object') {
        module.exports = domtoimage; // eslint-disable-line no-undef
    } else {
        global.domtoimage = domtoimage;
    }

    // support node and browsers
    const getComputedStyle = (global && global.getComputedStyle) || (window && window.getComputedStyle) || globalThis.getComputedStyle;
    const atob = (global && global.atob) || (window && window.atob) || globalThis.atob;
  console.log(getComputedStyle(null));
  console.log(atob('ff'));
})();

nstuyvesant · 2023-04-26T20:47:15Z

I took out global on line 1 and this on line 1392. Unfortunately, still getting the error about Node undefined. I assume I'll also get the error for window as well on 50 and 51 if that one were cleared.

IDisposable · 2023-04-26T20:48:40Z

That's annoying at best. Rollup's assumption this is a module is at the root of the issue. Tweaks to come, thanks for testing

nstuyvesant · 2023-04-26T20:49:42Z

Wish I could say the conversion to vite is ready...

IDisposable · 2023-04-26T21:06:32Z

So this looks promising https://jsfiddle.net/mjhr2nf1/ aka https://rollupjs.org/repl/?version=3.12.1&shareable=JTdCJTIyZXhhbXBsZSUyMiUzQW51bGwlMkMlMjJtb2R1bGVzJTIyJTNBJTVCJTdCJTIyY29kZSUyMiUzQSUyMihmdW5jdGlvbiUyMCgpJTIwJTdCJTVDbiUyMCUyMCUyMCUyMCd1c2UlMjBzdHJpY3QnJTNCJTVDbiUyMCUyMCUyMCUyMGNvbnN0JTIwRUxFTUVOVF9OT0RFJTIwJTNEJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMCh0eXBlb2YlMjBOb2RlJTIwISUzRCUzRCUyMCd1bmRlZmluZWQnJTIwJTNGJTIwTm9kZS5FTEVNRU5UX05PREUlMjAlM0ElMjB1bmRlZmluZWQpJTIwJTdDJTdDJTIwMSUzQiU1Q24lMjAlMjAlMjAlMjBjb25zdCUyMGdldENvbXB1dGVkU3R5bGUlMjAlM0QlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwKHR5cGVvZiUyMGdsb2JhbCUyMCElM0QlM0QlMjAndW5kZWZpbmVkJyUyMCUzRiUyMGdsb2JhbC5nZXRDb21wdXRlZFN0eWxlJTIwJTNBJTIwdW5kZWZpbmVkKSUyMCU3QyU3QyU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjAodHlwZW9mJTIwd2luZG93JTIwISUzRCUzRCUyMCd1bmRlZmluZWQnJTIwJTNGJTIwd2luZG93LmdldENvbXB1dGVkU3R5bGUlMjAlM0ElMjB1bmRlZmluZWQpJTIwJTdDJTdDJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMGdsb2JhbFRoaXMuZ2V0Q29tcHV0ZWRTdHlsZSUzQiU1Q24lMjAlMjAlMjAlMjBjb25zdCUyMGF0b2IlMjAlM0QlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwKHR5cGVvZiUyMGdsb2JhbCUyMCElM0QlM0QlMjAndW5kZWZpbmVkJyUyMCUzRiUyMGdsb2JhbC5hdG9iJTIwJTNBJTIwdW5kZWZpbmVkKSUyMCU3QyU3QyU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjAodHlwZW9mJTIwd2luZG93JTIwISUzRCUzRCUyMCd1bmRlZmluZWQnJTIwJTNGJTIwd2luZG93LmF0b2IlMjAlM0ElMjB1bmRlZmluZWQpJTIwJTdDJTdDJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMGdsb2JhbFRoaXMuYXRvYiUzQiU1Q24lMjAlMjBjb25zb2xlLmxvZyhFTEVNRU5UX05PREUpJTNCJTVDbiUyMCUyMGNvbnNvbGUubG9nKGdldENvbXB1dGVkU3R5bGUpJTNCJTVDbiUyMCUyMGNvbnNvbGUubG9nKGF0b2IpJTNCJTVDbiU3RCkoKSUzQiUyMiUyQyUyMmlzRW50cnklMjIlM0F0cnVlJTJDJTIybmFtZSUyMiUzQSUyMm1haW4uanMlMjIlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyb3V0cHV0JTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlN0QlMkMlMjJ0cmVlc2hha2UlMjIlM0F0cnVlJTdEJTdE

nstuyvesant · 2023-04-26T21:15:20Z

It does!

IDisposable · 2023-04-26T21:16:27Z

Looks like you could tweak rollup to know that this has a value using moduleContext https://rollupjs.org/configuration-options/#context but I'm going to ship a 3.1.6 with code matching the fiddle now

IDisposable · 2023-04-26T21:41:23Z

v3.1.6 released

nstuyvesant · 2023-04-26T21:45:29Z

Thank you! I just updated it and rollup is now working!

nstuyvesant mentioned this issue Apr 22, 2023

fix(window undefined): check to see if window is undefined #134

Closed

nstuyvesant changed the title ~~Window is not defined~~ Node and window is not defined when building @carbon/charts Apr 26, 2023

nstuyvesant closed this as completed Apr 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node and window is not defined when building @carbon/charts #133

Node and window is not defined when building @carbon/charts #133

nstuyvesant commented Apr 22, 2023

IDisposable commented Apr 25, 2023

IDisposable commented Apr 26, 2023 •

edited

Loading

nstuyvesant commented Apr 26, 2023

IDisposable commented Apr 26, 2023

nstuyvesant commented Apr 26, 2023 •

edited

Loading

IDisposable commented Apr 26, 2023 •

edited

Loading

IDisposable commented Apr 26, 2023

IDisposable commented Apr 26, 2023

nstuyvesant commented Apr 26, 2023

IDisposable commented Apr 26, 2023

nstuyvesant commented Apr 26, 2023

IDisposable commented Apr 26, 2023

nstuyvesant commented Apr 26, 2023

IDisposable commented Apr 26, 2023

IDisposable commented Apr 26, 2023

nstuyvesant commented Apr 26, 2023

Node and window is not defined when building @carbon/charts #133

Node and window is not defined when building @carbon/charts #133

Comments

nstuyvesant commented Apr 22, 2023

Use case: description, code

Expected behavior

Actual behavior (stack traces, console logs etc)

Library version

Browsers

IDisposable commented Apr 25, 2023

IDisposable commented Apr 26, 2023 • edited Loading

nstuyvesant commented Apr 26, 2023

IDisposable commented Apr 26, 2023

nstuyvesant commented Apr 26, 2023 • edited Loading

IDisposable commented Apr 26, 2023 • edited Loading

IDisposable commented Apr 26, 2023

IDisposable commented Apr 26, 2023

nstuyvesant commented Apr 26, 2023

IDisposable commented Apr 26, 2023

nstuyvesant commented Apr 26, 2023

IDisposable commented Apr 26, 2023

nstuyvesant commented Apr 26, 2023

IDisposable commented Apr 26, 2023

IDisposable commented Apr 26, 2023

nstuyvesant commented Apr 26, 2023

IDisposable commented Apr 26, 2023 •

edited

Loading

nstuyvesant commented Apr 26, 2023 •

edited

Loading

IDisposable commented Apr 26, 2023 •

edited

Loading