search

anandthakker / doiuse

:bomb: Lint CSS for browser support against caniuse database.
MIT License
1.23k stars 50 forks source link

Add support for all CSS features #159

Closed RJWadley closed 1 year ago

RJWadley commented 1 year ago

This pull request adds a script that gets updates the database and creates test and implementation stubs for any new CSS features. I've used these stubs to add support for every CSS feature in the caniuse-lite database.

Since this package seems abandoned, I've published these changes on npm as @rjwadley/doiuse. I'll be maintaining the package there until either I or someone else can maintain it here.

This PR also depends on #157, since the changes in that pull request made this script possible. That PR's changes are included in this one. That also means the diff isn't very useful right now. For a better view of the changes, here's a more focused diff with only the relevant changes.

New Features

Changes

Fixes

8

76

90

99

100

102

107

139

140

142

143

144

145

155

Supersedes

148

156

RJWadley commented 1 year ago

When #157 was merged it was merged by rebase, which meant all those commits appeared twice in this PR. As such, I've had to also rebase and force-push this branch and my other branches to remove the duplicates.

Whomever merges this, please just use a standard merge commit to prevent also breaking my other work. Thanks!

clshortfuse commented 1 year ago

Hi! Can you rebase?

Also it's best if the changes are a bit more atomic for better review.

I'm okay with multiple commits but the descriptions should be semantic/conventional. In other words separated by intent (fix, feat, docs, test, etc). For example, see https://gist.github.com/joshbuchea/6f47e86d2510bce28f8e7f42ae84c716 I'm not picky about intent names, but some at-a-glance commit descriptions would be helpful.

I hope this doesn't deter you from contributing and I really appreciate the effort you've put in. I know organizing and squashing git commits can be tedious.

Thanks again!

RJWadley commented 1 year ago

No worries. I'm not too experienced with rebase, but I'll see what I put together for ya.

If you'd like to enforce that sort of style, you might should set up commit linting and update CONTRIBUTING.md, since the repo doesn't really have a history of using semantic commits before you.

RJWadley commented 1 year ago

@clshortfuse I've done a rebase and cleaned things up best I can. I can also bring the version back to 6.0.0 if you want, since doiuse hasn't released that yet.

clshortfuse commented 1 year ago

Thanks! It's much easier to read now. I'm also pushing changes, so apologies for breaking the rebase 😅

The commit changes shouldn't affect package.json, or package-lock.json unless there's something specifically in those new versions that is required to run. It'll make more sense when we do a one shot update of all dependencies or individual ones (more than one of this PR's commit is updating caniuse).

You should also not touch CHANGELOG.md because your commit details should have what I would need to compile into the published version. Basically changes for git users are in the commits. Changes for npm users are in CHANGELOG.md. It's only when I make the packaged version that I make the modification to CHANGELOG.md. That allows me to cherry-pick certain things and even support multiple versions (eg: backport something to the 5.x branch).

So, when I review, I'm just looking at what folders were touched and if tests pass. I don't want to worry about runtime issues if you're only touching /test and /data. I look more closely at changes done to /lib and /utils.

In the end, this will land as one commit feat: add more css support. That's the at-a-glance description. In the details you can explain (if you want) that we're going to use a script to generate the feature list and compile tests off that.

I think with the changelog/package.json changes we should be good to merge. I'll push another commit that does those changes (including the contributor change =) )

RJWadley commented 1 year ago

I went ahead and dropped the changelog & package stuff. That commit also removed the travis badge, since CI runs on github actions now. I'll leave removing that up to you as well.

I made the feature script update caniuse, so that package got updated a lot while I was iterating. Sorry if that made things harder to read.

Would you prefer PRs only contain a single commit? I can totally do that if that's what you prefer. Or were you planning to squash this yourself when you merged?

clshortfuse commented 1 year ago

I was going to squash it into a single commit since each commit is based on work of the previous and stays within the same intention of adding support for all CSS features.

I'm okay with multiple commits generally and squash them before commit, though one with this many file changes can get tricky to review. But that's for future reference.

It seems there are still package.json changes. If squashing it into one commit helps you find where it's still making those changes, that's fine as well.

RJWadley commented 1 year ago

The remaining package.json changes are:

Would you like both of these reverted?

clshortfuse commented 1 year ago

Sorry, I wasn't notice they were essential. For future reference, a commit that just updates package.json by itself would probably be better. The language of the commit doesn't matter as much.

I'll go ahead and merge it. I have some internal changes and I want to make this part of 6.0.0. Thanks again for your work!