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

Add linting task to check IE compatibility #18774

Merged
merged 2 commits into from Nov 27, 2019

Conversation

@tellthemachines
Copy link
Contributor

tellthemachines commented Nov 27, 2019

Description

Added a script to lint built JS files checking for any ES > 5 that might break on IE11.
Added task to run script in Travis.

How has this been tested?

Tested locally by running npm run build followed by npm run lint-es5-only. On latest master, no errors should appear.
Checking out 69e7fc7060651c65b394944bda00759e232ade33 (or any commit before hex-rgb was removed) and re-running the above commands should result in an error traceable to the hex-rgb package.

Screenshots

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
@@ -178,6 +178,7 @@
"fixtures:generate": "npm run fixtures:server-registered && cross-env GENERATE_MISSING_FIXTURES=y npm run test-unit",
"fixtures:regenerate": "npm run fixtures:clean && npm run fixtures:generate",
"lint": "concurrently \"npm run lint-js\" \"npm run lint-pkg-json\" \"npm run lint-css\" \"npm run lint-types\"",
"lint-es5-only": "npx eslint --parser-options=ecmaVersion:5 --no-eslintrc --no-ignore ./build/**/*.js",

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 27, 2019

Contributor

I think we should inline this into the Travis config. It's not really something developers will run locally and npx and eslint are not direct, local dependencies.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Nov 27, 2019

Author Contributor

Hmm good point. Although in theory devs could use it locally to check if new dependencies are IE-compatible, it'll probably never happen 😁

This comment has been minimized.

Copy link
@aduth

aduth Dec 12, 2019

Member

Noting two things:

  • npx here would be redundant anyways, since NPM will include ./node_modules/.bin in the PATH when running any script. So the line here would work fine (the same) if running eslint directly.
  • If ESLint is not a defined dependency, then we shouldn't have confidence that it would be installed in Travis either. I have a feeling this is only incidentally working because it's some transitive dependency of another package (probably one of the ESLint plugins which are defined as dependencies).

This comment has been minimized.

Copy link
@epiqueras

epiqueras Dec 12, 2019

Contributor

But npx installs it if it's not there.

This comment has been minimized.

Copy link
@aduth

aduth Dec 12, 2019

Member

But npx installs it if it's not there.

Ah, you're right! I always forget that part of it.

@tellthemachines tellthemachines merged commit 8f739f2 into master Nov 27, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@tellthemachines tellthemachines deleted the try/linting-es6-in-built-files branch Nov 27, 2019
@aduth aduth mentioned this pull request Dec 4, 2019
6 of 6 tasks complete
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.