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 victory-vendor package for d3 dependencies #2204

Merged
merged 76 commits into from
May 10, 2022
Merged

Conversation

becca-bailey
Copy link
Contributor

@becca-bailey becca-bailey commented Apr 26, 2022

This PR addresses the ESM-only package issues from d3 packages discussed in #2124

We take a two-path approach in that ESM uses the real ESM-only packages and CJS uses our vendored (transpiled) package versions.

Tasks

Stuff we still need to do:

  • Validate victory-native still works as expected.
  • Confirm storybook works as expected.
  • Docs: the build fails for victory-vendor
  • Docs: A build failure doesn't fail CI
  • Review and remove all TODOs.
  • Additional CHANGELOG.md notes.
  • Fix it.skip()-ed Karma tests.
  • Our yarn nps lint.docs command does not presently run in CI. It has a unique constraint in that both the root and docs dependencies need to be installed for it to succeed. We can either fix / enable here or maybe ticket for later.

Work

New stuff:

Chores:

  • Upgrade various babel deps that needed to be modernized.
  • Refactor and simplify babel and webpack configs.
  • Added more helper development commands in CONTRIBUTING.md

Fixes / infra:

  • Shore up our import strategy for Jest and Karma. Now we only use built lib code in tests (ESM in Karma, CJS in Jest). Unfortunately this touches a lot of files and we end up with a big PR.

Release notes

We'll be publishing a new package victory-vendor, so I'm not sure if there are extra kinks in the release script there.

Also, for this release, since we've got a new vendor dependency scheme in the package, there are potentially some unforeseen issues that we won't be able to fully suss out until we fully release.

@github-actions github-actions bot temporarily deployed to staging-2204 May 5, 2022 20:46 Inactive
@github-actions github-actions bot temporarily deployed to staging-2204 May 5, 2022 20:53 Inactive
Copy link
Contributor Author

@becca-bailey becca-bailey left a comment

Choose a reason for hiding this comment

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

These changes look good to me! @ryan-roemer You can go ahead and merge whenever you are ready, and then we can get this package released.

@becca-bailey
Copy link
Contributor Author

becca-bailey commented May 6, 2022

I'm not sure why there is a diff in Chromatic, but there are no visible changes. I'm assuming there is just some microscopic difference because of a slight change to the d3 scale that it is picking up on. I'll approve those changes.

@github-actions github-actions bot temporarily deployed to staging-2204 May 6, 2022 22:14 Inactive
@becca-bailey
Copy link
Contributor Author

becca-bailey commented May 6, 2022

@ryan-roemer I'm reviewing changes in chromatic, and there do appear to be a couple places where commenting out those scales changed things. I think we needed one of those log scales? There are also a few mysterious markup changes in the svg container, so I can investigate those a little more.

Screen Shot 2022-05-06 at 3 18 07 PM

@ryan-roemer
Copy link
Member

@becca-bailey Sounds good and good to know those Chromatic tests can flag this for us!!!

I'm holding off pending your investigation.

@github-actions github-actions bot temporarily deployed to staging-2204 May 10, 2022 17:06 Inactive
@github-actions github-actions bot temporarily deployed to staging-2204 May 10, 2022 18:41 Inactive
@github-actions github-actions bot temporarily deployed to staging-2204 May 10, 2022 19:32 Inactive
@github-actions github-actions bot temporarily deployed to staging-2204 May 10, 2022 19:56 Inactive
@becca-bailey
Copy link
Contributor Author

From what I can tell, the axis label changes introduced in this PR have to do with changes introduced to the scale.tickFormat function in the d3-scale package. Here is more info about d3's default formatting. https://github.com/d3/d3-format/blob/main/README.md#locale_formatPrefix

This showed up as a diff in Chromatic, and users might notice some minor changes to the way log scale values are formatted by default. We will include some guidance in the release notes for the next version about Victory's tickFormat prop and how users can override the d3 default with their own formatting functions.

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.

2 participants