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.

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

Added caniuse-lite browsers release date check #267

Merged
merged 5 commits into from Jun 13, 2018
Merged

Added caniuse-lite browsers release date check #267

merged 5 commits into from Jun 13, 2018

Conversation

@An-Tu
Copy link
Contributor

@An-Tu An-Tu commented Jun 2, 2018

No description provided.


it('checkCanIUse doesn\'t throw warning', () => {
browserslist('last 2 versions')
expect(console.warn).toHaveBeenCalledTimes(1)

This comment has been minimized.

@ai

ai Jun 2, 2018
Member

It is better to check exact text (maybe Jest will throw warning too)

This comment has been minimized.

@ai

ai Jun 2, 2018
Member

Also, we need to check yarn/npm detection. You can do it by mocking fs methods.

node.js Outdated
var halfYearAgo = new Date(
today.getFullYear(),
today.getMonth() - MONTH_TO_UPDATE_CANIUSE,
today.getDate()

This comment has been minimized.

@ai

ai Jun 2, 2018
Member

It could generate invalid date if today in 31 days of month and month - 6 will have only 28/30 days.

It is better just use Date.now() - 6 * 30 * 24 * 60 * 60 * 1000

node.js Outdated
var hasYarnLock = false
eachParent(__filename, function (dir) {
var yarnLock = path.join(dir, 'yarn.lock')
if (isFile(yarnLock)) {

This comment has been minimized.

@ai

ai Jun 2, 2018
Member

It is better to check package.json and yarn.lock in the same dir

node.js Outdated
})

var packageManager = hasYarnLock ? 'yarn upgrade' : 'npm update'
console.warn('\x1b[1m!!![Browserslist] WARN: ' +

This comment has been minimized.

@ai

ai Jun 2, 2018
Member

Not every terminal has color support

@ai
Copy link
Member

@ai ai commented Jun 2, 2018

Thanks, good PR. But you need a few fixes (I added comments).

…w we look for package.json too. Update release test. Years test - added now's method to the mocked Date
@An-Tu
Copy link
Contributor Author

@An-Tu An-Tu commented Jun 2, 2018

Hi. I corrected and I wait for comments =)

@ai
Copy link
Member

@ai ai commented Jun 2, 2018

  1. You need to fix Travis CI (reduce the size or increase limit).
  2. By some reason, all tests print the warning.
  3. Warning text will be better without !!!. It is not so urgent.
@An-Tu
Copy link
Contributor Author

@An-Tu An-Tu commented Jun 2, 2018

Is need to remove warning from all tests? Because "years" and "since" tests use releseDate which older than six month.

@ai
Copy link
Member

@ai ai commented Jun 2, 2018

Yeap. Right now test output is noisy. You can fix this tests.

@An-Tu
Copy link
Contributor Author

@An-Tu An-Tu commented Jun 2, 2018

Ok, I got it

@An-Tu
Copy link
Contributor Author

@An-Tu An-Tu commented Jun 3, 2018

Unfortunately, I don't know how to reduce the limit :(

@ai
Copy link
Member

@ai ai commented Jun 3, 2018

Awesome 👍

I put your name to the Cult of Martians task, but will merge it and release only after PiterCSS (need more time for slides).

@An-Tu
Copy link
Contributor Author

@An-Tu An-Tu commented Jun 3, 2018

Hooray, Thak you! It was my first PR to the open-source project.
See you at PiterCSS =)

@ai
Copy link
Member

@ai ai commented Jun 3, 2018

Find me on piterCSS. I will give a sockets to you.

@ai ai changed the base branch from master to v4 Jun 13, 2018
@ai ai merged commit efe2654 into browserslist:v4 Jun 13, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ai ai mentioned this pull request Jun 13, 2018
ai added a commit that referenced this pull request Jun 21, 2018
* Added caniuse-lite browsers release date check

* Node.js checkCanIUse - was amended time constant to update canius, now we look for package.json too. Update release test. Years test - added now's method to the mocked Date

* Remove warning from test output. Increase size limit
@gaearon
Copy link

@gaearon gaearon commented Mar 22, 2019

This warning is pretty confusing. I encountered it when building a CRA project: facebook/create-react-app#6708.

It is completely unactionable to our users who don't even know what Browserslist or caniuse-lite is. The suggestion to add a new version might actually subtly break their setups (this has happened before). In general it's better if a package doesn't assume it's a top-level dependency, and that the consumer is using it directly. This is not always the case.

@ai
Copy link
Member

@ai ai commented Mar 22, 2019

This warning is pretty confusing

Sure. What is your suggestion?

The suggestion to add a new version might actually subtly break their setups

Yeap, npm update update everything, which is a little dangerous.

Do you know how to update only specific npm package deeply in an application?

@AndrewSouthpaw
Copy link

@AndrewSouthpaw AndrewSouthpaw commented Jun 27, 2019

The suggestion in facebook/create-react-app#6708 was to delete that entry out of the (npm|yarn).lock file and then re-running the install (npm install / yarn), and that seemed to fix the issue. Might be enough to suggest as much with this warning.

@csimi
Copy link

@csimi csimi commented Jan 30, 2020

It's especially weird if you're targeting ES5 (as a library or due to company requirements). Absolutely unactionable since updating will do nothing in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants