Node and window is not defined when building @carbon/charts about dom-to-image-more HOT 16 CLOSED

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

from dom-to-image-more.

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

from dom-to-image-more.

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

from dom-to-image-more.

Here are steps to reproduce...

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

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

3. Navigate to the core package

cd packages/core

4. 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?

from dom-to-image-more.

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.

from dom-to-image-more.

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.

from dom-to-image-more.

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

from dom-to-image-more.

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?

from dom-to-image-more.

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'));
})();

from dom-to-image-more.

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.

from dom-to-image-more.

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

from dom-to-image-more.

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

from dom-to-image-more.

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

from dom-to-image-more.

It does!

from dom-to-image-more.

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

from dom-to-image-more.

v3.1.6 released

from dom-to-image-more.