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

Framework: Introduce custom ESLint rules, including Lodash import restrictions #3015

Merged
merged 4 commits into from
Feb 8, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Feb 2, 2016

This pull request seeks to include the newly published eslint-plugin-wpcalypso package in introducing custom ESLint rules to the Calypso project. Currently, the plugin only contains a single rule: no-lodash-import. This rule intends to prevent an undesirable increase in bundle size as a consequence of importing modules from the root Lodash package, which has been fixed once before in #735. New ESLint rules can be added to the eslint-plugin-wpcalypso package as needs arise.

Testing instructions:

Ensure that Lodash replacements are valid.

Verify that make lint passes (reflected by CircleCI tests passing).

Test that introducing an import from the root Lodash package results in an ESLint error:

  1. Edit any file to include import { compose } from 'lodash';
  2. Save the file
  3. Run ./node_modules/.bin/eslint [filename] in your Terminal
  4. Note that the file fails linting with the appropriate error message

Depending on how you use ESLint in your development workflow, you may need to install the eslint-plugin-wpcalypso package globally (using the -g flag). This will be the case if you have installed the base eslint package globally.

@aduth aduth added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 2, 2016
@gwwar
Copy link
Contributor

gwwar commented Feb 5, 2016

👍 Tested that Calypso runs normally, that warnings appear, and that you're prevented from committing changes with a root lodash import. We may want to update the error message to explain that this is restricted to help reduce bundle size.

Feel free to 🚢 after a rebase.

lint

nocommit

@gwwar gwwar added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 5, 2016
@aduth
Copy link
Contributor Author

aduth commented Feb 5, 2016

We may want to update the error message to explain that this is restricted to help reduce bundle size.

I dunno, I kinda like the brevity of ESLint's built-in rules, and tried to adopt a similar feel to the message, in that the text itself should briefly describe what is incorrect, and the rule key should lead you to a more thorough explanation of why it's incorrect.

Per recent discussions, we should also separately consider whether it makes sense to bring the eslint-plugin-wpcalypso into the Calypso repository itself (treating Calypso as a mono-repo, see #2247).

@aduth aduth force-pushed the fix/eslint-no-lodash-import branch from d0f11be to 0388bca Compare February 5, 2016 20:27
@aduth aduth force-pushed the fix/eslint-no-lodash-import branch from 0388bca to b41af2f Compare February 8, 2016 15:49
aduth added a commit that referenced this pull request Feb 8, 2016
Framework: Introduce custom ESLint rules, including Lodash import restrictions
@aduth aduth merged commit aac0692 into master Feb 8, 2016
@aduth aduth deleted the fix/eslint-no-lodash-import branch February 8, 2016 16:06
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.

3 participants