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

test: Add support for Node < 6 #3442

Merged
merged 1 commit into from
Feb 22, 2019

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Feb 12, 2019

Node < 6 doesn’t support destructuring.

This updates the tests (and other code) to not use it, thus removing the reason why #3428 was set to >= 6.0.0

@@ -1,6 +1,6 @@
'use strict';
const path = require('path');
const {browsers} = require('..');
const browsers = require('..').browsers;
Copy link
Contributor Author

@ExE-Boss ExE-Boss Feb 12, 2019

Choose a reason for hiding this comment

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

The fact that this depends on the whole package also causes a syntax error in non‑browser files to cause the tests to fail with an unhelpful error message (see #2330).

@Elchi3 Elchi3 added infra 🏗️ Infrastructure issues (npm, GitHub Actions, releases) of this project linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files. labels Feb 13, 2019
@ddbeck
Copy link
Collaborator

ddbeck commented Feb 13, 2019

Should we change anything to support Node versions before 6? All of the Node releases <6 have already reached end of life and Node 6 is EOL in a little less than two months (source)

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Feb 13, 2019

Well, it should still improve performance in modern Node given that this will stop depending on object destructuring to import test functions into the linter.

Adding syntactic support for Node < 6 is just a nice bonus.

@ddbeck
Copy link
Collaborator

ddbeck commented Feb 18, 2019

OK, since there's a justification apart from supporting older Node versions, this works for me.

I'm going to request Florian's review though because I think infra changes deserve a second or third look, depending on how you count.

@ddbeck ddbeck requested a review from Elchi3 February 18, 2019 10:34
Copy link
Member

@Elchi3 Elchi3 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 to me! 👍

Questions before I merge this:
Do we also want to lower the minimum node version now?
If so, do we also check what minimum node versions our (dev)Dependencies have?

@ExE-Boss
Copy link
Contributor Author

Can we get this merged?

@Elchi3
Copy link
Member

Elchi3 commented Feb 22, 2019

Yes, as this is not actually changing the minimum node version. I'll leave my questions unanswered then.

@Elchi3 Elchi3 merged commit 8d84cf5 into mdn:master Feb 22, 2019
@ExE-Boss ExE-Boss deleted the test/no-destructuring-imports branch February 22, 2019 16:58
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Feb 22, 2019

I think the version should be answered in an issue first (#3509).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra 🏗️ Infrastructure issues (npm, GitHub Actions, releases) of this project linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants