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

TypeError: node.addEventListener is not a function #5998

Closed
sbrl opened this issue Jan 19, 2019 · 6 comments · Fixed by #6046
Closed

TypeError: node.addEventListener is not a function #5998

sbrl opened this issue Jan 19, 2019 · 6 comments · Fixed by #6046
Milestone

Comments

@sbrl
Copy link

sbrl commented Jan 19, 2019

Expected Behavior

I should be able to use window.addEventListener as normal.

Current Behavior

Chart.JS overrides window.addEventListener() with a completely different implementation with a different method signature!

Possible Solution

Wrap Chart.JS (and all sub-files!) in a SEAF (self-executing anonymous function) to avoid polluting the global scope.

Steps to Reproduce (for bugs)

An 'live demo' is not useful here. Instead, I can provide detailed steps to reproduce, and a link tot he commit in the source tree of my open-source project: https://github.com/ConnectedHumber/Air-Quality-Web/tree/4a9d67e2924edc511bca1b0530a3370a3b20af5d (note that there may be probably are additional commits after this, so if you git clone you'll want to git checkout the exact commit).

  • I'm using rollup as my JS build system, with the ES6 module syntax.
  • I'm installing packages from NPM - including Chart.JS

I've got an index.mjs like this:

"use strict";

import '../css/main.css';

import MyProjectClass from './MyProjectClass.mjs';

window.addEventListener("load", function(_event) {
	window.project = new MyProjectClass();
	window.map.setup();
});

...the Chart.JS import is buried in the application. It looks like this (in one of the sub-files):

import Chart from 'chart.js';

I get the following output from rollup on the command-line:

client_src/js/index.mjs → app/app.js...
(!) postcss plugin: The onwrite hook used by plugin postcss is deprecated. The generateBundle hook should be used instead.
(!) Circular dependency: node_modules/moment/src/lib/create/from-anything.js -> node_modules/moment/src/lib/create/valid.js -> node_modules/moment/src/lib/create/utc.js -> node_modules/moment/src/lib/create/from-anything.js
(!) Circular dependency: node_modules/moment/src/lib/units/month.js -> node_modules/moment/src/lib/moment/get-set.js -> node_modules/moment/src/lib/units/month.js
(!) Circular dependency: node_modules/moment/src/lib/moment/get-set.js -> node_modules/moment/src/lib/units/year.js -> node_modules/moment/src/lib/moment/get-set.js
(!) Circular dependency: node_modules/moment/src/lib/create/local.js -> node_modules/moment/src/lib/create/from-anything.js -> node_modules/moment/src/lib/locale/locales.js -> node_modules/moment/src/lib/locale/base-config.js -> node_modules/moment/src/lib/units/week.js -> node_modules/moment/src/lib/create/local.js
(!) Circular dependency: node_modules/moment/src/lib/create/local.js -> node_modules/moment/src/lib/create/from-anything.js -> node_modules/moment/src/lib/locale/locales.js -> node_modules/moment/src/lib/locale/base-config.js -> node_modules/moment/src/lib/units/week.js -> node_modules/moment/src/lib/units/week-calendar-utils.js -> node_modules/moment/src/lib/create/local.js
(!) Circular dependency: node_modules/moment/src/lib/create/from-string-and-format.js -> node_modules/moment/src/lib/create/from-string.js -> node_modules/moment/src/lib/create/from-string-and-format.js
(!) Circular dependency: node_modules/moment/src/lib/create/local.js -> node_modules/moment/src/lib/create/from-anything.js -> node_modules/moment/src/lib/create/from-string-and-array.js -> node_modules/moment/src/lib/create/from-string-and-format.js -> node_modules/moment/src/lib/create/from-string.js -> node_modules/moment/src/lib/create/from-array.js -> node_modules/moment/src/lib/create/local.js
(!) Circular dependency: node_modules/moment/src/lib/duration/constructor.js -> node_modules/moment/src/lib/duration/valid.js -> node_modules/moment/src/lib/duration/constructor.js
(!) Circular dependency: node_modules/moment/src/lib/duration/create.js -> node_modules/moment/src/lib/duration/constructor.js -> node_modules/moment/src/lib/duration/valid.js -> node_modules/moment/src/lib/duration/create.js
(!) Circular dependency: node_modules/moment/src/lib/duration/create.js -> node_modules/moment/src/lib/units/offset.js -> node_modules/moment/src/lib/duration/create.js
(!) Circular dependency: node_modules/moment/src/lib/moment/add-subtract.js -> node_modules/moment/src/lib/duration/create.js -> node_modules/moment/src/lib/units/offset.js -> node_modules/moment/src/lib/moment/add-subtract.js
created app/app.js in 2.5s

The error in the Firefox Developer Tools comes with a stack trace. Here it is:

TypeError: node.addEventListener is not a function[Learn More] platform.dom.js:127:1

    addEventListener platform.dom.js:127
    <anonymous> index.mjs:9 

.....that platform.dom.js is part of Chart.JS. As I have source maps enabled, it tells me where that file is located: http://[::1]:40482/node_modules/chart.js/src/platforms/platform.dom.js

Context

I'm using Rollup as my build system to create a dynamic map (with LeafletJS) of some data. Line graphs are a part of this - so, after a quick search of npm, I decided to bring in Chart.JS to do the heavy lifting with respect to displaying the graphs etc. Unfortunately, this had an adverse affect on the rest of my application because Chart.JS pollutes the global scope.

Environment

@sbrl sbrl added the type: bug label Jan 19, 2019
@simonbrunel
Copy link
Member

We don't override addEventListener, it's a local function (not exported) but your rollup process gathers everything in the same scope, thus our addEventListener version become part of your (global) scope and is used instead of the window one. If you wrap your own code in a IIFE, then I "think" it should work as expected (but I could be wrong).

Wrap Chart.JS (and all sub-files!) in a SEAF ... to avoid polluting the global scope

That's already the case if you use our UMD build (dist/Chart.min.js), wrapped in a IIFE and which is, IMO, the best way to include Chart.js. Note that this file will become the main entry point starting 2.8, so require('chart.js') will use dist/Chart.js instead of src/chart.js.

Anyway, I'm not against renaming our local methods addEventListener and removeEventListener to addListener and removeListener, which should solve your issue and would prevent any conflict with the global scope.

@sbrl
Copy link
Author

sbrl commented Jan 20, 2019

Huh, interesting! Thanks for the clarification, @simonbrunel!

Is there any way to get rollup to separate scope automatically? I'm rather new to using a JS build system :P

I see. Looks like importing Chart.min.js works great as a workaround! Thanks for that 😺

According to #5978 that you've linked to, moment.js is required for time support on axes, but isn't included in the bundle. Does this mean I need to npm install --save moment.js to use this functionality?

@benmccann
Copy link
Contributor

moment is included if you're using npm even after that PR is merged

@sbrl
Copy link
Author

sbrl commented Jan 23, 2019

Cool, thanks :-)

@sbrl
Copy link
Author

sbrl commented Feb 6, 2019

Thanks so much, @simonbrunel 😺

@simonbrunel
Copy link
Member

Thanks @kurkle actually :)

@simonbrunel simonbrunel added this to the Version 2.8 milestone Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants