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

feat: unicorn rules #35

Merged
merged 10 commits into from Jul 4, 2019

Conversation

Projects
None yet
7 participants
@manniL
Copy link
Member

commented Apr 24, 2019

resolves #34

Please go through the changes and leave feedback ☺️

I tried to be not that strict to avoid errors in existing code bases and with 3rd party libs (e.g. by not preventing abbreviations)

EDIT: Though the last test is failing, the rules do work 🤔

image

@manniL manniL requested review from pi0, pimlie, galvez, aldarund, Atinux, clarkdo, kevinmarrec and nuxt/core-team Apr 24, 2019

@pimlie

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Are these rules meant for server side / node only or also for client / browser code?

@manniL

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

@pimlie Ideally it should be codebase agnostic

@pimlie
Copy link
Contributor

left a comment

Nice work, overall looks like valuable additions!

I think we have to be careful with enforcing new ES features, imo they are not always preferred due to polyfills needed in older browsers and sometimes there just dont provide more readability. Although that might be personal preference.

The following rules might be a valuable addition as well:

  • no-hex-escape (at least if we are talking about strings / characters)
  • no-process-exit (for nuxt command's, because its better to use the exit pkg instead so you are sure the stdout is flushed before exiting)
Show resolved Hide resolved index.js Outdated
Show resolved Hide resolved index.js Outdated
Show resolved Hide resolved index.js Outdated
Show resolved Hide resolved index.js Outdated
Show resolved Hide resolved index.js
Show resolved Hide resolved index.js Outdated
Show resolved Hide resolved index.js Outdated
Show resolved Hide resolved index.js
Show resolved Hide resolved index.js Outdated
Show resolved Hide resolved index.js Outdated
@clarkdo
Copy link
Contributor

left a comment

The unicorn rules are more aggressive since it prefers ES2019 syntax.

This is good for Nuxt core code like renderer, builder which are normally running under a high version of Node.js.

But for vue client entrypoint, app and components like nuxt, nuxt-child which will be transpiled by babel, using modern syntax may not have benefits in bundle size (bigger size due to traispiled code and polyfill) and performance (traispiled code may be slower than old native implementation).

So maybe we can filter out some modern syntax precedence rules like unicorn/prefer-spread.

@manniL

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

Please all keep in mind that all the rules can be deactivated easily. We should generally think about if we want to enforce a more strict config which users can disable through .eslintrc (etc.) if they want or if we provide a more relaxed one where users can add rules.

@manniL

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

@pimlie

Regarding the suggested rules:

The following rules might be a valuable addition as well:

  • no-hex-escape (at least if we are talking about strings / characters)

I think hex regex expressions are quite popular and, at least for me, easier to read. But I have no problem with adding the rule if it gives a larger benefit.


  • no-process-exit (for nuxt command's, because its better to use the exit pkg instead so you are sure the stdout is flushed before exiting)

Would it be better for userland code as well, e.g. nuxt modules?

@galvez

galvez approved these changes May 13, 2019

Copy link
Member

left a comment

At first glance, and upon further investigation of the unicorn rules not included in the diff, it seems mostly reasonable to me. It's impossible to give final feedback before we actually see what the Nuxt codebase will look like once these rules have been applied via --fix. Once we see the final result of the change, I sense we might want to tweak a thing here or there.

@galvez

This comment has been minimized.

Copy link
Member

commented May 13, 2019

@Atinux @manniL @pi0 @pimlie Perhaps this is how something like this should be accomplished: instead of a PR to eslint-config first, we start by manually appending all proposed rules to the eslint config in nuxt/nuxt.js, then we can have a much, much clearer picture of the changes involved by analyzing a PR that has a lot of code linted with --fix. Anyway, just a thought :)

@manniL

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

@galvez I like the idea! Maybe we could even make it a test/a pipeline step for the package which we could check out? 🤔

@pi0

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Some rules are not limited to nuxt/nuxt.js repo and are not testable. ESLint validates a parsable valid code against some optionised rules :)

@pi0
Copy link
Member

left a comment

Please remove no-for-loop and prefer-query-selector. They are highly optionised and there is no valid technical reason to disallow them.

@pimlie

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

I would like to re-iterate that imo lint rules should safe guard code style commonality but only when / where it really matters. We should leave some room for personal preferences as most code in all the nuxt repositories is in practice mostly maintained by only one developer so why care whether someone uses eg \d vs [0-9] or getElementById vs querySelector

@aldarund

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@pimlie but personal preferences could be changed via manually adjusting rules

@manniL

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

I'm totally fine with that approach (using standard-style + rules which are not based on personal preference and actually improve the code quality ☺️). That's why I asked in #35 (comment)

@manniL manniL requested review from pi0, clarkdo, pimlie and galvez and removed request for clarkdo May 14, 2019

@pimlie

pimlie approved these changes May 14, 2019

Copy link
Contributor

left a comment

Thanks @manniL, LGTM

@pi0

pi0 approved these changes May 14, 2019

Copy link
Member

left a comment

Thanks @manniL

@manniL

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

Final call for reviews, feedback, critics and so on!

I'd merge it next week so we push out a v1 soon ☺️

@Atinux

Atinux approved these changes Jun 14, 2019

@manniL manniL force-pushed the feat/unicorn-rules branch from cfc73dc to d890105 Jul 4, 2019

@manniL manniL merged commit 03bb05f into master Jul 4, 2019

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details

@manniL manniL deleted the feat/unicorn-rules branch Jul 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.