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

chore: add Node.js 8 and 10 #100

Merged
merged 2 commits into from
May 5, 2018
Merged

chore: add Node.js 8 and 10 #100

merged 2 commits into from
May 5, 2018

Conversation

DanielRuf
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented May 4, 2018

Codecov Report

Merging #100 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #100   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines         194    194           
  Branches       33     33           
=====================================
  Hits          194    194

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 532cf88...f8978f2. Read the comment docs.

@iamkun
Copy link
Owner

iamkun commented May 4, 2018

@DanielRuf what's this pr for?

@DanielRuf
Copy link
Contributor Author

@DanielRuf what's this pr for?

This PR is to add Node.js 8 and 10 to the Travis builds so we can run the unit tests on these versions too and catch Node.js 8 and 10 bugs earlier and make sure that also these stable and LTS releases are supported.

@DanielRuf
Copy link
Contributor Author

@iamkun
Copy link
Owner

iamkun commented May 4, 2018

Yes, I know what's this config used for.

I'm not sure if we need it. Actually we have all these tests in the past. You could see a travis-deploy-once used in the .travis.yml at that time.

Just wondering if our unit test runs perfectly on Node6, a lower node version, dose that mean it shoud be good on 8 and 10?

Plus, will this expand the total CI process time?

@DanielRuf
Copy link
Contributor Author

Plus, will this expand the total CI process time

No, not in general. These do normally use a test matrix.

I'm not sure if we need it. Actually we have all these tests in the past. You could see a travis-deploy-once used in the .travis.yml at that time.

Why not? There can be always changes in Node.js which affect libraries.

You could see a travis-deploy-once used in the .travis.yml at that time.

You can constrain it to a specific Node.js version / test.

@DanielRuf
Copy link
Contributor Author

Regarding different Node.js versions and testing: webpack/watchpack#80 (comment)

Also older versions (4 is EOL) often break and dependencies are often not compatible anymore (eg puppeteer requires Node.js 6+).

@DanielRuf
Copy link
Contributor Author

@iamkun
Copy link
Owner

iamkun commented May 4, 2018

All right, may be we could give it a try

@iamkun
Copy link
Owner

iamkun commented May 5, 2018

Let's give it a try.

@iamkun iamkun merged commit ee485f1 into iamkun:master May 5, 2018
@DanielRuf DanielRuf deleted the chore/add-nodejs-8-10 branch May 5, 2018 08:19
@xx45
Copy link
Contributor

xx45 commented May 7, 2018

🎉 This PR is included in version 1.5.20 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants