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

Removing source files for never released v2 tokens #640

Closed
wants to merge 1 commit into from

Conversation

lukasoppermann
Copy link
Contributor

@lukasoppermann lukasoppermann commented May 3, 2023

Summary

V2 tokens were never really released so they don't need to be deprecated.

Todo before merging

  • @rezrah is using the tokens in primer/brand, so we can't delete them before primer/brand is updated
  • Release v8 tokens
  • Replace all v2 with v8 tokens (e.g. PVC, PCSS, dotcom)

@lukasoppermann lukasoppermann requested a review from a team as a code owner May 3, 2023 16:21
@lukasoppermann lukasoppermann requested review from a team, simurai and colebemis May 3, 2023 16:21
@changeset-bot
Copy link

changeset-bot bot commented May 3, 2023

⚠️ No Changeset found

Latest commit: abc07ad

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@lukasoppermann lukasoppermann requested review from langermank and rezrah and removed request for colebemis May 3, 2023 16:21
@lukasoppermann
Copy link
Contributor Author

Hey @rezrah are you using the tokens or the scripts related to build:tokens from the package.json in brand?

Are you using anything from the build.js file for brand?

@lukasoppermann lukasoppermann marked this pull request as draft May 5, 2023 10:36
Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

V2 tokens were never really released so they don't need to be deprecated.

Yeah, we never "officially released" the v2 tokens. But we already started to use them in PVC, PCSS and even on dotcom.

It should be fine to remove the v2 tokens, since hopefully all cases have a fallback (e.g. hard coded 1px). Although if feels more correct to:

  1. Release v8 tokens
  2. Replace all v2 with v8 tokens
  3. Remove v2 tokens from Primitives.

/cc @langermank

@langermank
Copy link
Contributor

langermank commented May 10, 2023

@simurai @lukasoppermann I already removed all references to v2 in dotcom/PCSS/PVC/PRC and pointed them to v8 (the secret pre-release) 😄 the only thing holding this back is Brand, but we haven't heard back from Reza yet if that's an issue.

@lukasoppermann
Copy link
Contributor Author

@simurai @lukasoppermann I already removed all references to v2 in dotcom/PCSS/PVC/PRC and pointed them to v8 (the secret pre-release) 😄 the only thing holding this back is Brand, but we haven't heard back from Reza yet if that's an issue.

I actually talked to @rezrah and they are using v2 tokens. So I think we need to do it like this:

  1. Release v8
  2. Have brand update their tokens to v8
  3. Remove v2 tokens

Alternatively they may be able to just pull in a specific tag so taht we can move forward and they stay on the old version of the package.

@rezrah
Copy link
Contributor

rezrah commented May 10, 2023

EDIT after seeing @lukasoppermann' closely-timed update above. That plan looks good to me, thanks 🙇


Thanks for the ping, and sorry for the late response.

I'm likely missing some background on this change, could I ask why these need to be removed right now? V2 tokens are still a core dependency for Primer Brand, so I'm a little worried that removing this will effectively cut the library off from future updates. I'd also speculate that migration effort from v2 to v8 is quite substantial, which we haven't factored in this quarter.

If you need to go-ahead with this, no worries.. we can look to prioritise upgrading with urgency 👍 ... alternatively, you could leave them in for now and I can remove these files after the Primer Brand upgrade is complete.

@lukasoppermann
Copy link
Contributor Author

@rezrah I think we don't need to rip it out immediately.

How about this:

  1. We don't compile tokens anymore and remove them (compiled tokens) from the npm package
  2. We remove the build script from package.json
  3. We move build.js to scripts/buildv2.js (@rezrah you would need to update to use this file than, but it is a tiny change)
  4. We remove any workflows that check v2 tokens adn remove v2 builds, etc. from workflows

This gives you some time to update brand and once you have it updated we can remove the tokens & pipeline.

@lukasoppermann lukasoppermann deleted the rm-v2-tokens branch April 9, 2024 10:24
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.

None yet

4 participants