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

Code: Refactor export default {} objects to one-off exports. #650

Closed
2 tasks
ryan-roemer opened this issue Jul 13, 2017 · 4 comments · Fixed by #1907
Closed
2 tasks

Code: Refactor export default {} objects to one-off exports. #650

ryan-roemer opened this issue Jul 13, 2017 · 4 comments · Fixed by #1907
Assignees
Labels
Note: Good first issue 🤩 Good issue for new contributors

Comments

@ryan-roemer
Copy link
Member

Background

Tree shaking relies on named specific exports and can't pick from mega objects. Throughout our victory repos code we have patterns like:

export default {
  circle(x, y, size) {
    // CODE
  },

  square(x, y, size) {
    // CODE
  },

  plus(x, y, size) {
    // CODE
  },

  star(x, y, size) {
    // CODE
  }
};

which we should refactor to:

export const circle = (x, y, size) => {
  // CODE
};

export const square = (x, y, size) => {
  // CODE
};

export const plus = (x, y, size) => {
  // CODE
};

export const star = (x, y, size) => {
  // CODE
};

Task

  • Identify all export default {} that are actually individual objects / vars we should individually export
  • Leave things like prop objects or legitimate first class objects that should be exported as a whole.
@ryan-roemer ryan-roemer added Note: Good first issue 🤩 Good issue for new contributors improvement labels Jul 13, 2017
@aronstrandberg
Copy link
Contributor

Hi @ryan-roemer! We're a small group of Computer Science students from KTH (Stockholm, Sweden). We'd like to tackle this issue as part of a school assignment to contribute to an open source project. Can you assign me to it? Also, is there anything else we should know before we begin, that's not stated in the CONTRIBUTING files? :-)

@ryan-roemer
Copy link
Member Author

ryan-roemer commented Jul 15, 2021

@beccanelson and I were chatting Victory + Lodash + tree-shaking today, and in checking out https://github.com/FormidableLabs/victory/blob/main/packages/victory-core/src/victory-util/helpers.js#L307-L327 I noticed a huge default export that can't be tree-shaken:

export default {
  omit,
  getPoint,
  scalePoint,
  getPadding,
  getDefaultStyles,
  getStyles,
  evaluateProp,
  evaluateStyle,
  degreesToRadians,
  radiansToDegrees,
  getRadius,
  getPolarOrigin,
  getRange,
  createAccessor,
  modifyProps,
  getCurrentAxis,
  reduceChildren,
  isHorizontal,
  isTooltip
};

Maybe we want to do a quick assessment of size impact and number of files where we could slim things down?

@becca-bailey
Copy link
Contributor

@ryan-roemer @boygirl I can re-open this issue - it looks like there are several files in victory-util that will need to be updated.

Based on the initial research I have done, it sounds like we can namespace these files, and still support tree-shaking. This means that instead of import Helpers from './helpers' we can use import * as Helpers from './helpers'. https://stackoverflow.com/a/45746950

As far as I know, we can also use @babel/plugin-proposal-export-namespace-from to export * as Helpers from './helpers' in the index so we can still import { Helpers } from 'victory-core'.

Other than changing a few imports (and adding function exports to the utils), this shouldn't require many changes to the code itself. Does this sound reasonable?

@becca-bailey becca-bailey reopened this Jul 16, 2021
@becca-bailey becca-bailey self-assigned this Jul 16, 2021
@boygirl
Copy link
Contributor

boygirl commented Jul 19, 2021

@beccanelson That sounds like a good course of action. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Note: Good first issue 🤩 Good issue for new contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants