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

ESLint cleanup #204

Merged
merged 6 commits into from
Oct 13, 2019
Merged

ESLint cleanup #204

merged 6 commits into from
Oct 13, 2019

Conversation

raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Jun 20, 2019

  • 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

src/common/modulemapper.js Outdated Show resolved Hide resolved
@raphinesse
Copy link
Contributor Author

I've updated my changes to not use ES6 or beyond and furthermore add a linter check to avoid such mistakes in the future.

Copy link
Member

@erisu erisu left a 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)

@raphinesse
Copy link
Contributor Author

Q: Is there a reason why the test/build.js file was moved outside of the testing directory structure?

Yes, the reason was that it is not a test but a build-tool and the linting rules in build-tools were a fit for it. That being said, I see your concern and I'm not 100% happy with the move either. But I also didn't want to have three different kinds of linting rule files in test/.

@raphinesse raphinesse merged commit 7bff8e8 into apache:master Oct 13, 2019
@raphinesse raphinesse deleted the eslint-cleanup branch October 13, 2019 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants