-
Notifications
You must be signed in to change notification settings - Fork 832
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
ESLint cleanup #204
ESLint cleanup #204
Conversation
raphinesse
commented
Jun 20, 2019
•
edited
Loading
edited
- Add proper ESLint configuration
- Fix ESLint violations instead of ignoring them
- Remove unused function
- Remove node-only code
- This was previously required for being able to run the tests on Node.js. But since we now run them in an actual browser, this isn't required anymore.
- Check that we only use ES5 in our browser code
- Required for Android 4.4 (cordova-android) and IE 11 (cordova-windows) support
dce1463
to
e19ab8a
Compare
I've updated my changes to not use ES6 or beyond and furthermore add a linter check to avoid such mistakes in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything is OK but only had one question and concern that is not blocking for this repo.
Q: Is there a reason why the test/build.js
file was moved outside of the testing directory structure?
IMO: Since cordova-js
is not an npm package deployed publicly, maybe this is OK.
Concern: If I saw this in a repo that was deployed as an npm package though, I would find it as a tedious task to ensure that all testing files (spread though out src code) were not deployed with the production package. (.npmignore
list)
Yes, the reason was that it is not a test but a build-tool and the linting rules in |