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

Use eslint-eslines as the orchestrator for linting tools #17896

Closed
wants to merge 1 commit into from

Conversation

oandregal
Copy link
Contributor

@oandregal oandregal commented Sep 7, 2017

We have reached peak modularity in our linting tools -eslint, eslint-config-wpcalypso, eslint-plugin-wpcalypso, eslines, eslint-eslines. Updating and releasing new versions of any package is taking more effort than it should, and considering that we do this for wp-calypso and any other project that uses the same patterns, the process doesn't scale very well.

This proposal aims to reduce the effort by using eslint-eslines as an orchestrator package that keeps every other package in sync.

Testing

Check that CircleCI test PRs run as expected:

Locally:

  • Clone this branch.
  • Add some changes that break some lint rule (remove spaces, for example), and commit: the expected result is that the git commit hook reports the linting error and prevents you from committing them.
  • You may want to commit them anyway (use --no-verify) and push to a remote branch to double-check that CircleCI reports the same.
  • Repeat the experiment but with changes that don't break any linting rule (for example, remove the debug statements in some file). The expected result is that the git commit hook doesn't report any linting error and allows you to commit them.
  • If you push them to a remote branch, CircleCI shouldn't report any linting issues.

@oandregal oandregal self-assigned this Sep 7, 2017
@matticbot matticbot added the [Size] S Small sized issue label Sep 7, 2017
@oandregal oandregal changed the title Compact linting tools Use eslint-eslines as the orchestrator for linting tools Sep 7, 2017
@oandregal
Copy link
Contributor Author

Jarda: I cannot test the prettier magic; would you mind giving this PR a try and confirm that it doesn't break anything for prettier?

Andrew, Jarda: this alternative makes our current setup easy to manage and I would prefer it to #17868 if we don't have any concerns. Let me know your thoughts.

@jsnajdr
Copy link
Member

jsnajdr commented Sep 7, 2017

I verified that the Prettier filtering works correctly in this branch, too.

I'm a bit worried by the fact that eslint and other packages are no longer direct dependencies of wp-calypso and that they might end up at unexpected places in the node_modules hierarchy. Instead of:

./node_modules/eslint
./node_modules/eslint-config-wpcalypso

they can be in

./node_modules/eslint-eslines/node_modules/eslint
./node_modules/eslint-eslines/node_modules/eslint-config-wpcalypso

Will an editor integration find the eslint module in the wp-calypso directory? Will eslint find the Calypso config and the rules from the wpcalypso plugin?

Maybe the dependencies are actually guaranteed to be in the top-level directory if there are no conflicting packages that would require different versions of the same package. I don't know. But with my limited knowledge of NPM details, this looks like a potential problem.

Another solution that might work: declare eslint et al. as peer dependencies of eslint-eslines with exact versions, and declare them as loose-versioned dependencies in wp-calypso. That should guarantee that the right version is always installed without updating dozen places, and that the packages are available as direct dependencies of wp-calypso.

@oandregal
Copy link
Contributor Author

The relevant docs.

I could prepare a reproducible use case that makes this fail (defining fail as that an ESLint version different from the one in eslint-eslines is installed at the node-modules top-level): just add a package that includes an older version of ESLint as a dependency (not dev-dependency) and it'll be added as the top-level ESLint. I couldn't manage to install v4 after trying different things (ordering the packages, naming, etc).

@oandregal
Copy link
Contributor Author

I'm going to merge #17868 first. By pinning ESLint and eslines major versions as peer dependencies (see) eslint-eslines will only need to be released after a major version change - which is a once a year event- and that makes things simpler.

Current situation still requires some bookkeeping and makes difficult to sync projects, so I'm curious to explore the orchestrator idea - I'll post shortly an alternate approach in this same branch.

@oandregal
Copy link
Contributor Author

FWIW, what I've tried unsucessfully:

In Calypso, import eslint-eslines as eslint (even the same version). In eslint-eslines:

  • importing eslint from GitHub, to avoid the circular dependency. It still has it, and it doesn't work.

  • removing ESLint as a dependency and have it as a submodule instead. For some reason, and although the top-level eslint points to Automattic/eslint-eslines#try/compact-linting-tools the v3.8.1 is installed. Haven't investigated why.

It was worth a shot, but I'd put this in the backburner at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Size] S Small sized issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants