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

New/compat package #3106

Merged
merged 2 commits into from
Oct 16, 2019
Merged

New/compat package #3106

merged 2 commits into from
Oct 16, 2019

Conversation

molant
Copy link
Member

@molant molant commented Oct 10, 2019

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

Short description of the change(s)

Fix #3035

@molant molant force-pushed the new/compat-package branch from 67253dc to a85642e Compare October 10, 2019 22:05
@molant
Copy link
Member Author

molant commented Oct 10, 2019

@atopal here's the PR for the package to query the browser compat data. Could you please take a look at the README.md to see if this is something that could be useful for other projects and if there is something you think we should/could add?

@molant
Copy link
Member Author

molant commented Oct 10, 2019

@Elchi3 in case you are interested as well on this tool.

@molant molant force-pushed the new/compat-package branch from a85642e to bf96406 Compare October 10, 2019 22:41
@molant
Copy link
Member Author

molant commented Oct 11, 2019

Code coverage issues in utils 🤦‍♂️

@molant molant force-pushed the new/compat-package branch from bf96406 to 84eb70a Compare October 11, 2019 00:29
sarvaje
sarvaje approved these changes Oct 11, 2019
View reviewed changes
Copy link
Contributor

@sarvaje sarvaje left a comment

Choose a reason for hiding this comment

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

LGTM, just a NIT, but nothing blocking.

packages/hint-compat-api/src/utils/browsers.ts Outdated
import { UnsupportedBrowsers } from '@hint/utils/dist/src/compat';
import { getFriendlyName } from '@hint/utils/dist/src/compat/browsers';
import { UnsupportedBrowsers } from '@hint/utils-compat-data';
import { getFriendlyName } from '@hint/utils-compat-data';
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Use a single import.

antross
antross approved these changes Oct 14, 2019
View reviewed changes
Copy link
Member

@antross antross left a comment

Choose a reason for hiding this comment

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

Looks good! I left some comments about minor tweaks to the README, but that's it.

packages/utils-compat-data/README.md Outdated
# Utils compat data (`@hint/utils-compat-data`)

This package allows you to easily query the data from [mdn-browser-compat-data][]
to know about the support status of a feature.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
to know about the support status of a feature.
to learn about the support status of a feature.

packages/utils-compat-data/README.md Show resolved Hide resolved
packages/utils-compat-data/README.md Outdated

### `isSupported`

Query MDN for support of CSS or HTML features.
Copy link
Member

Choose a reason for hiding this comment

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

We should call out that this requires all provided browsers to have support in order to return true. If any single provided browser is not supported this will return false.

@molant molant force-pushed the new/compat-package branch from 84eb70a to 89441d0 Compare October 14, 2019 20:44
@molant molant requested review from antross and sarvaje October 14, 2019 20:45
@molant
Copy link
Member Author

molant commented Oct 14, 2019

Comments addressed in case you want to take a second round. Otherwise I'll merge this tomorrow.

@molant molant force-pushed the new/compat-package branch from 89441d0 to 3dd1acf Compare October 14, 2019 20:47
sarvaje
sarvaje suggested changes Oct 14, 2019
View reviewed changes
packages/utils-compat-data/package.json Outdated
"timeout": "1m"
},
"dependencies": {
"@types/parse5": "^5.0.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this ok as dependency or should it be a devDependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

we might not even need a lot of this stuff. I think I copy pasted from the original utils and didn't do a lot of clean up.
Looking forward to #3104 to get merged

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed all the unused dependencies

@molant molant force-pushed the new/compat-package branch from 3dd1acf to 28ee784 Compare October 14, 2019 22:29
@molant molant requested a review from sarvaje October 14, 2019 22:30
@molant
Copy link
Member Author

molant commented Oct 14, 2019

Coverage failing for Windows 😭

@antross
Copy link
Member

antross commented Oct 15, 2019

Coverage failing for Windows 😭

The difference appears to be coming from packages/utils/src/npm.ts:

Linux CI:
npm.ts | 87.69 | 96 | 61.54 | 87.69 |... 37,143,148,154 |
Windows CI:
npm.ts | 87.69 | 88 | 61.54 | 87.69 |... 37,143,148,154 |

Specifically I suspect it's this check on line 92:

    ${!isWindows && global ? 'sudo ' : ''}${command}
            manually to install all the packages.`);

@molant molant force-pushed the new/compat-package branch from 28ee784 to 6e32ecd Compare October 15, 2019 16:18
@molant
Copy link
Member Author

molant commented Oct 15, 2019

Let's see if a good old istanbul ignore next does the trick... That said, we should increase the coverage of this package...

@molant molant force-pushed the new/compat-package branch 3 times, most recently from a3ddad4 to c10c658 Compare October 16, 2019 17:37
@antross antross merged commit fa357fd into webhintio:master Oct 16, 2019
antross pushed a commit that referenced this pull request Oct 16, 2019
@molant @antross
Breaking: Extract compat information to new package
56788d5 
Fix #3035
Close #3106
@molant molant deleted the new/compat-package branch October 23, 2019 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antross antross antross approved these changes

@utsavized utsavized Awaiting requested review from utsavized

@sarvaje sarvaje Awaiting requested review from sarvaje

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Separate the compat data into its own utils package
3 participants
@molant @antross @sarvaje