Skip to content

Conversation

@BenLorantfy
Copy link
Contributor

After #116, if you omit the label from your dataset because you don't need it, it fails silently and all the datasets are combined into one.

This is a small PR to output a warning if a user tries to add datasets without keys, similar to how React outputs a warning when you omit a key.

screen shot 2017-05-23 at 8 38 26 pm

This was referenced May 24, 2017
src/index.js Outdated
const shouldWarn = !currentDatasetKeys.every(d => typeof d !== "undefined") || !nextDatasetKeys.every(d => typeof d !== "undefined");
const newDatasets = nextDatasets.filter(d => currentDatasetKeys.indexOf(this.props.datasetKeyProvider(d)) === -1);

if (shouldWarn && !this.hasWarned) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should only occur if you are not in a production build. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can I check if production vs. development?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this will work unless you have a better method https://stackoverflow.com/a/25922668

@kumarharsh
Copy link

+1 this should be merged!

@BenLorantfy
Copy link
Contributor Author

BenLorantfy commented Jul 7, 2017

Wasn't sure the best way to check the enviroment. There's this method https://stackoverflow.com/a/35470995 but that seems like it will only work if the user sets the NODE_ENV enviroment variable. There's also this method https://stackoverflow.com/a/25922668 which seems a little hacky but will probably work most often. Any suggestions on how to do this would be appreciated.

@kumarharsh
Copy link

React itself uses NODE_ENV to check it. See invariant

@BenLorantfy
Copy link
Contributor Author

BenLorantfy commented Feb 3, 2018

@jerairrest , @benmccann Took me almost a year but I re-based and added the production check. Also added some unit tests. How's it now? 🙂

@BenLorantfy
Copy link
Contributor Author

@jerairrest just pinging

@jerairrest jerairrest merged commit a99556b into reactchartjs:master Apr 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants