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

ES6 - assignments to exported members #1543

Closed
kum-deepak opened this issue Aug 11, 2019 · 10 comments
Closed

ES6 - assignments to exported members #1543

kum-deepak opened this issue Aug 11, 2019 · 10 comments
Labels

Comments

@kum-deepak
Copy link
Collaborator

kum-deepak commented Aug 11, 2019

Ref #1520

Rollup generated js does not allow reassigning any exported members. Currently test cases fail that use reassigning:

  • disableTransitions (one spec)
  • redrawAll - two specs put a spy on this call (which try to replace this function)
@gordonwoodhull
Copy link
Contributor

There will probably be a long tail of things that no longer work because they rely on reassignment.

There are a few global settings on the dc namespace. We might have to make these into global setters.

Not sure what to do about spies.

@kum-deepak
Copy link
Collaborator Author

My thinking is not firmed yet. However my current thinking is like the following:

  • Currently there are few settings which are somewhat global
  • Package all these settings into a class - say Config or DcConfig
  • Attach an instance of this object (with default values) to the Chart Group
  • A chart will always know which chart group it is linked to
  • The config can be assigned to a chart group or to a chart
  • If no config has been assigned to a chart (which is likely to be the case), it will use config from the chart group

However before this we will need to discuss how much refactoring and breakage of existing API is acceptable.

@kum-deepak
Copy link
Collaborator Author

As of now commented out these 3 specs via b741b1a

@gordonwoodhull gordonwoodhull changed the title ES6 - fix specs ES6 - assignments to exported members Aug 13, 2019
@kum-deepak
Copy link
Collaborator Author

I have been reading about ES6 and tree shaking. It seems a proper structure would be like following:

  • No top level exported API should manipulate state or depend on state.
  • No mutable state should be a top level exported symbol.
  • Mutable state should be part of an Object (preferably of a defined Class). If not possible than wrapped inside a class as static members (e.g., utils.uniqueId).
  • All stateless helper methods can become top level exports (e.g., utils.arraysEqual)
  • Some larger, limited use, methods can go to their own modules (e.g., utils.toHierarchy)

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Aug 13, 2019

Seems like there needs to be a third level: look for config

  1. on chart, then if there's none set
  2. on chart group, or finally
  3. on dc itself, as demonstrated in Refactoing #1553.

Or does the third one violate "No mutable state should be a top level exported symbol"?

I hope not, since it would be handy. I guess if all else fails we could make dc.config immutable and only contain defaults, but that's not as convenient for users.

@gordonwoodhull
Copy link
Contributor

Probably would not want to cascade individual settings, just look for a config object at each level, which will have some value for every setting.

@kum-deepak
Copy link
Collaborator Author

Makes sense.

@kum-deepak
Copy link
Collaborator Author

I think we can close this task for this version. More changes than the current state would cause interface breakage.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Oct 19, 2019

Agreed. I looked over src/core and any function anyone would want to replace is under the filters, utils, or config namespace and therefore still assignable.

Removing the spies on dc.redrawAll to close this out

@gordonwoodhull
Copy link
Contributor

spies removed in 78b4c13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants