3.0.0 #40
3.0.0 #40
Conversation
.appveyor.yml
Outdated
|
|
||
| environment: | ||
| matrix: | ||
| - nodejs_version: 4.0 |
There was a problem hiding this comment.
Shouldn't it be
- nodejs_version: 4
- nodejs_version: 6
- nodejs_version: 8
- nodejs_version: 9
?
There was a problem hiding this comment.
Node is backwards compatible. This means Node v0.12 code works in Node v4, v9, etc. Therefore, since the lowest supported version is v4, this is the version that is tested, insuring the others will work as well.
There was a problem hiding this comment.
Node is backwards compatible. This means Node v0.12 code works in Node v4, v9, etc.
The backwards compatibility on node mostly concerns on V8 and Node.js API backwards compatibility. However, every SEMVER-MAJOR of node might introduces breaking changes due to third_party dependencies.
There is an obscure example of breakage on the daily-used \s pattern:
/\s/.test("\u180e")
// returns true on node.js 4.0
// returns false on node.js recent versionsThe breakage is due to Unicode 6.3 recategorize U+180E as format control. While node.js 4.0 depends on Unicode earlier than 6.3, later version will depends on later version since the latest ECMAScript always use latest Unicode standard.
Not to detour, given the routine of the plugin is pretty simple and straightforward (we don't depends on Unicode Character Property in our regex), it is acceptable to test against node.js 4 only.
A practical node.js version principle should be testing against the minimum postcss supporting node.js version.
Another question is would nodejs_version: 4.0 be resolved as 4.0.x only on appveyor/travis? We should test against the latest nodejs 4.
There was a problem hiding this comment.
I agree with the practical approach to test against the minimum Node version that postcss is supporting, which is v4. Will this be acceptable?
I have updated .appveyor.yml to match .travis.yml.
| - yarn add coveralls | ||
| - cat ./coverage/lcov.info | ./node_modules/.bin/coveralls | ||
| node_js: | ||
| - 4 |
| @@ -1,21 +0,0 @@ | |||
| { | |||
There was a problem hiding this comment.
Shouldn't yaspeller be added to postcss-plugin-boilerplate?
There was a problem hiding this comment.
I will add yaspeller
CHANGELOG.md
Outdated
| @@ -1,3 +1,15 @@ | |||
| # Changes to postcss-font-family-system-ui | |||
|
|
|||
| #### 2.1.4 (2018-01-17) | |||
There was a problem hiding this comment.
should be 3.x.x for later manual change.
There was a problem hiding this comment.
I will fix the CHANGELOG
index.js
Outdated
| 'Oxygen', | ||
| 'Ubuntu', | ||
| 'Cantarell', | ||
| 'Fira Sans', |
There was a problem hiding this comment.
I am considering remove Fira Sans as Firefox OS is dead.
There was a problem hiding this comment.
This is fine. I will remove it from this PR, and add the change to the CHANGELOG
index.js
Outdated
|
|
||
| // system-ui and fallbacks match | ||
| const whitespace = '[\\n\\r\\x09\\x20]'; | ||
| const systemUiMatch = new RegExp(`(^|,|${whitespace})(?:${whitespace}*system-ui${whitespace}*)(?:,${whitespace}*(?:${systemUiFamily.join('|')})${whitespace}*)?(,|$)`, 'i'); |
There was a problem hiding this comment.
When system-ui is used as a fallback generic font, the replace phrase will remove the extra whitespace before comma.
Expected:
"Droid Sans", system-ui, foo
Actual
"Droid Sans",system-ui, foo
There was a problem hiding this comment.
I will add a test for this and fix it. Thanks!
index.js
Outdated
| ]; | ||
|
|
||
| // system-ui and fallbacks match | ||
| const whitespace = '[\\n\\r\\x09\\x20]'; |
There was a problem hiding this comment.
Newline also includes \f, see https://www.w3.org/TR/css-syntax-3/#newline-diagram
There was a problem hiding this comment.
I’ll add this
|
Hi @JLHwung, I have addressed all of the changes you requested and rebased the commit :) |
|
@jonathantneal 🤔 Looks like we need |
|
Well, the odd thing is that it’s already there: 8df7366#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R46 |
|
Looks like we’re good to merge? 😄 |
|
I am okay with most revision except for a question on nodejs version settings. See https://github.com/JLHwung/postcss-font-family-system-ui/pull/40/files#r162538816 |
|
I’ve addressed that and agreed with the practical approach. Let me know if that’ll work for us. |
|
3.0.0 is released. 🎉 |
This PR refactors postcss-font-family-system-ui to use the postcss-boilerplate plugin boilerplate.
@std/esmforrollup..jsextension, following npm's proposal for supporting Modules in Node.cc @RyanZim