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

Fix issue that broke themes #142 #146

Merged
merged 1 commit into from
Aug 16, 2018

Conversation

nlfurniss
Copy link
Collaborator

@nlfurniss nlfurniss commented Aug 15, 2018

Issue

This commit fixes an issue that arose after I removed jQuery from the addon. It also adds a test to prevent regressions.

To ensure everything works as it used to, I also ran the new test against v0.6.0 (pre jQuery removal) and it passes.

Current solution

This PR adds deepmerge as a dependency to take the place of jQuery.extend(true, ...);. As it is released as an ES6 module, it was necessary to use a transform to import it into ember-highcharts.

Downside to my approach

Using the es6 transform requires ember-cli >= 2.16. Ember-cli 2.16 was introduced 10 months ago, so while there might be some users who cannot take advantage of this change, presumably the gross majority won't find this a problem.

Other possible solutions

  • Use ember-auto-import
    This is a great addon, but if the consuming app has a CSP that disallows eval, it won't work.

  • Inline deepmerge
    Seemed hacky, but has no ember-cli support issues.

@RobbieTheWagner
Copy link
Collaborator

@nlfurniss can we use ember-copy to make a deep copy? That is what you are looking for right, to deep copy something?

@nlfurniss
Copy link
Collaborator Author

No, because we're not trying to copy options, but trying to merge the theme and chartOptions

@RobbieTheWagner
Copy link
Collaborator

@nlfurniss how about Object.assign? There has to be something we can use that doesn't require another package.

@nlfurniss
Copy link
Collaborator Author

nlfurniss commented Aug 15, 2018

assign doesn't do deep merging. It's fine for simple and flat objects, but chart themes and options are complex and deep.

@RobbieTheWagner
Copy link
Collaborator

RobbieTheWagner commented Aug 16, 2018

@nlfurniss can we use ember-cli-node-assets to import deepmerge then? It would be good to not require ES6 transforms. if we can, although it's not necessarily a deal breaker. I've done shims for things with it before, for example https://github.com/shipshapecode/ember-vivus/blob/master/index.js

@nlfurniss nlfurniss force-pushed the fix-options-deep-merge branch from af6a17e to 633bb3d Compare August 16, 2018 06:59
@nlfurniss
Copy link
Collaborator Author

nlfurniss commented Aug 16, 2018

@rwwagner90 good call with node-assets, didn't know about it.

Updated the implementation, should be good to go with no change to supported versions of ember-cli. The umd package is 567B when minified and gzipped, so not a large price to pay to restore this critical functionality.

@nlfurniss nlfurniss merged commit 773d144 into ahmadsoe:master Aug 16, 2018
@nlfurniss nlfurniss deleted the fix-options-deep-merge branch August 16, 2018 20:04
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.

Theme does not work as is worked before 0.7
2 participants