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

Add XYChart to @visx/visx #984

Conversation

ShaneHudson
Copy link

This PR relates to #974 to ensure XYChart is in the package that includes all other packages.

💥 Breaking Changes

  • none

🚀 Enhancements

  • Adds XYChart to the @visx/visx package.

📝 Documentation

  • none

🐛 Bug Fix

  • none

🏠 Internal

  • none

@ShaneHudson ShaneHudson force-pushed the shane--@visx/visx-include-xychart branch 2 times, most recently from 87740ec to 6a5f901 Compare December 29, 2020 12:32
@ShaneHudson ShaneHudson force-pushed the shane--@visx/visx-include-xychart branch from 6a5f901 to e246f92 Compare December 29, 2020 12:34
@ShaneHudson
Copy link
Author

The build failed, it says to regenerate types but not sure how.

@williaster
Copy link
Collaborator

@ShaneHudson thanks for the PR! ❤️ sorry for the review delay (holidays 🎁 ).

I think this is failing because some of the named exports from XYChart conflict with exports from different packages 😱 this is (surprisingly) the first time for name clashes, so may need to rethink the approach. I think probably the best thing is to move toward name-spacing the exports into separate files to avoid this. Need to be cognizant of breaking changes tho so I would propose:

  1. (this PR, no breaking changes): create a new src/xychart.ts file which has the export * from '@visx/xychart'; line and avoids name conflicts.
  2. (follow up PR, consistency with no breaking changes): create a new src/xxx.ts for each package, e.g., src/shape.ts would export * from '@visx/shape';. the src/index.ts is unchanged.
  3. (follow up PR, for breaking 2.0.0 release): remove src/index.ts and remove package.json main + module entries which reference this index.

I'll probably create an issue to track the multiple parts. If you want to combine 1 and 2 into this PR feel free else I'm happy to take a stab 🔜 . There's no planned 2.0.0 yet, but I'll start a project with known todo items for it.

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

Successfully merging this pull request may close these issues.

2 participants