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.

Sign up for GitHub

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

Jump to bottom

Add variable deprecation data and tests #971

Merged
merged 10 commits into from
Nov 4, 2019
Merged

Add variable deprecation data and tests #971

merged 10 commits into from
Nov 4, 2019

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Nov 1, 2019 •
edited
Loading

👀 Updated docs: contributing, deprecation data

This adds planned variable declaration messages for 14.0.0 (#925) and adds deprecation tests for variables in script/test-deprecations.js. The output looks like this (but with colored symbols!):

i no variables changed (latest -> 14.0.0)
i 11 variables to be deprecated in 14.0.0
i no selectors changed in bundle "primer" (latest -> 14.0.0)
i 6 selectors to be deprecated in 14.0.0

11 errors:
𐄂 variable "$status-pending" deprecated, but not removed
𐄂 variable "$repo-private-text" deprecated, but not removed
𐄂 variable "$repo-private-bg" deprecated, but not removed
𐄂 variable "$repo-private-icon" deprecated, but not removed
𐄂 variable "$marketingSpacers" deprecated, but not removed
𐄂 variable "$allSpacers" deprecated, but not removed
𐄂 variable "$exploregrid-item-border-radius" deprecated, but not removed
𐄂 variable "$stats-switcher-py" deprecated, but not removed
𐄂 variable "$stats-viewport-height" deprecated, but not removed
𐄂 variable "$min_tab_size" deprecated, but not removed
𐄂 variable "$max_tab_size" deprecated, but not removed

The errors come from new entries in deprecations.js, and the test script has been updated to look for selectors and/or variables arrays in any of the version deprecation "entries", e.g.

const versionDeprecations = {
  '14.0.0': [
    {selectors: ['.selector-a', '.selector-b'], message: 'These selectors have been deprecated.'},
    {variables: ['$variable-a', '$variable-b'], message: 'These variables have been deprecated.'}
  ]
}

The diff from one version to the next is determined from the keys of the dist/variables.json data generated in script/dist.js before publication, so script/test-deprecations.js should always be run on a fresh dist directory built by script/dist.js.

When this is merged into #925 with #968 (or if we merge #968 first, then update this branch) it should trigger a bunch of errors that we'll need to fix in the release branch. 🤞

@vercel
Copy link

vercel bot commented Nov 1, 2019 •
edited
Loading

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/primer/primer-css/ghltnvrla
🌍 Preview: https://primer-css-git-variable-deprecations.primer.now.sh

@shawnbot shawnbot mentioned this pull request Nov 1, 2019
Merged
1 task
simurai
simurai approved these changes Nov 4, 2019
View reviewed changes
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.

LGTM! 👍 Although one question:

When planning to delete a CSS selector or SCSS variable, you should:
2. Add it to [deprecations.js]:

Should deprecations.js get updated in the same PR where the variables/selectors get removed? One concern might be that because all PRs that have deprecations are based on master, they all try edit the same spot in deprecations.js. So when starting to merge those PRs into the release branch, there will be constantly merge conflicts. Maybe fine if there are not too many, but could also get annoying. I can think of 2 options:

  1. Do the "develop branch" idea. So that PRs (plus changes to deprecations.js) get merged as soon as they are ready. And new PRs get branched off that already include these changes to deprecations.js.
  2. Not worry about updating deprecations.js for every PR. Only once all PRs that go into a major release are merged, then update deprecations.js for everything. Kinda similar how we update CHANGELOG only "at the end".

docs/content/getting-started/contributing.md Outdated Show resolved Hide resolved
@shawnbot @simurai
fix typo
c6be855 
Co-Authored-By: simurai <simulus@gmail.com>
@shawnbot
Copy link
Contributor Author

shawnbot commented Nov 4, 2019 •
edited
Loading

Should deprecations.js get updated in the same PR where the variables/selectors get removed? One concern might be that because all PRs that have deprecations are based on master, they all try edit the same spot in deprecations.js. So when starting to merge those PRs into the release branch, there will be constantly merge conflicts. Maybe fine if there are not too many, but could also get annoying. I can think of 2 options:

I don't think we'll be deprecating things so often that we'll need to worry about merge conflicts in this file, but if we end up in that situation my vote would be for:

2. Not worry about updating deprecations.js for every PR. Only once all PRs that go into a major release are merged, then update deprecations.js for everything. Kinda similar how we update CHANGELOG only "at the end".

@shawnbot shawnbot merged commit 4fec939 into release-14.0.0 Nov 4, 2019
@shawnbot shawnbot mentioned this pull request Nov 5, 2019
Merged
19 tasks
@jonrohan jonrohan deleted the variable-deprecations branch March 26, 2021 16:51
@github-actions github-actions bot mentioned this pull request Dec 18, 2022
Open
This was referenced Dec 25, 2022
Open
Open
This was referenced Jan 3, 2023
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simurai simurai simurai approved these changes

@BinaryMuse BinaryMuse Awaiting requested review from BinaryMuse

Assignees
No one assigned
Labels
Tag: Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shawnbot @simurai