feat: Add @wordpress/browserslist-config package
#69
Conversation
Codecov Report
@@ Coverage Diff @@
## master #69 +/- ##
==========================================
+ Coverage 73.55% 73.62% +0.07%
==========================================
Files 23 24 +1
Lines 363 364 +1
Branches 76 76
==========================================
+ Hits 267 268 +1
Misses 79 79
Partials 17 17
Continue to review full report at Codecov.
|
|
Awesome, I’ll take a closer look when I have time 👍 |
| @@ -0,0 +1,17 @@ | |||
| # 2.0.0 | |||
|
|
|||
| - Transfer to @wordpress npm orginization | |||
| "devDependencies": { | ||
| "browserslist": "^2.11.3" | ||
| }, | ||
| "peerDependencies": { |
There was a problem hiding this comment.
Is it necessary since it is already listed as devDependencies? Or devDependencies should be dependencies? I never know :)
There was a problem hiding this comment.
As browserslist is only used in the Jest tests it only requires to be included in devDependencies, that said, here's why it is included:
I added it to peerDependencies in ntwb/browserslist-config-wordpress@8c6f1b1 because of eslint-plugin-node's no-unpublished-require rule.
I opened an issue a while ago here asking that devDependencies modules be ignored from these rules and the end result was not what I had hoped for, hence the addition of browserslist to peerDependencies works as a workaround.
These are the two ESLint errors thrown when browserslist is not included in peerDependencies
1:31 error "browserslist" is not found node/no-missing-require
1:31 error "browserslist" is not published node/no-unpublished-require
I'm going to remove it from peerDependencies and add an .eslintrc.json in the test folder to ignore those two eslint-plugin-node/recommended rules as suggested here which I think is not quite as hacky...
There was a problem hiding this comment.
I removed it 0dfaed0, will add the . eslintrc.json when adding ESLint in, I'm tempted to turn both those rules off by default config now.
| it( 'should not contain invalid queries', () => { | ||
| jest.doMock( '@wordpress/browserslist-config', () => require( '../index' ), { virtual: true }); | ||
|
|
||
| const result = browserslist(['extends @wordpress/browserslist-config']); |
There was a problem hiding this comment.
Spacing should be fixed to match coding styles.
There was a problem hiding this comment.
I'm going to skip this change, for now, we need to get ESLint setup
The blocker to this is getting the changes from https://make.wordpress.org/core/2017/09/07/proposal-for-js-standards-revision-removing-array-function-whitespace-exceptions/ merged.
In me hacking in ESLint to run over this PR both the following return no ESLint errors:
• const result = browserslist([ 'extends @wordpress/browserslist-config' ]);
• const result = browserslist(['extends @wordpress/browserslist-config']);
And this does:
• const result = browserslist( ['extends @wordpress/browserslist-config'] );
I'm not sure why space-in-parens ESLint rule is throwing that, I could look now but instead, I'll get it fixed over the weekend, or early next week, a public holiday here today so I'm going to head out for a bit 🏖
| @@ -0,0 +1,17 @@ | |||
| const browserslist = require( 'browserslist' ); | |||
There was a problem hiding this comment.
Yes, the whole file should follow WordPress spacing rules.
This pull request migrates the existing package from:
• npm module: https://www.npmjs.com/package/browserslist-config-wordpress
• GitHub repo: https://github.com/ntwb/browserslist-config-wordpress
to:
• npm module: https://www.npmjs.com/package/@wordpress/browserslist-config
• GitHub repo: https://github.com/WordPress/packages/tree/master/packages/browserslist-config
I'd originally planned on having npm migrate the package for my from my repo/account to @WordPress', though as no other modules are currently consuming this package it was simpler to update and move the code here and have it published as a new package.