- It's time, we want to be able to use more modern features, and now we're on node 14 min this makes sense
- @TODO: do this in eslint-plugin-ghost and add new rules to guard the new features
- we have these rules in place to force the developer to think about the
performance impact of their code
- unless they are enforced, it's easy for them to slip by and miss them
- this commit changes the warnings to errors, which will fail CI
- developers still have the ability to disable lines that flag the
linting in case there is a valid use-case
- We had an incident where a migration file was misnamed.
- This is caused by using slimer with spaces rather than hyphens
- We didn't imagine this case when writing the regex for matching filenames
- However, now we know it, it's easy to tweak the regex to match this case
- This requires adding an override to the one badly named file
- Keeping overrides inside the file means the context of why a rule isn't firing is present inline when modifying a file
- This makes it easier to maintain files, at the cost of needing to search to find all overrides
refs: https://github.com/TryGhost/Toolbox/issues/105
Lint rules prevent:
* Invalid naming conventions for new migrations
* Loop constructs in migrations - these should be used with caution
and are therefore a warning rule, use `// eslint-disable-next-line
no-restricted-syntax` to prevent this rule from firing where a loop is
required
* Returing within a loop - this is usually meant to be a
continue/break
* Multiple joins - these can be badly performing migrations, so should
be treated with caution, disable the rule for the line if the risk is
understood / the migration cannot be written without it
refs: https://github.com/TryGhost/Toolbox/issues/105
The idea here is to ensure we're at least warning on bad migrations patterns. If a pattern is already in use in an existing migration, we can use `eslint-disable-next-line no-restricted-syntax` to override it. For new migrations which still need these features this step will force the user to think about the performance of the construct they are using
refs f0242baf9f
- The index.js files in "input validators" are exposing an API and not doing anything else so these cases can be safely ignored
- Ideally we'd have a solution which would export all modules in the folder without having to add any more declarations, but that's for another time!
- we're now so close to having the server not require anything from the frontend... so close
- having these last few problems be visible is motivating
- should be able to upgrade it to an error soooon!
refs: bd4000e7f
- we can remove these again now that we've done the work to get rid of the two worst offenders: lib/common/events and urlservice
- happened quicker than I expected :D
- url service is responsible for many problem requires, and will be removed soon
- the same with lib/common/events
- excluding these leaves us with a handful of much harder to solve requires clearly indicated
- These rules are currently off. We turn them on whilst testing atm to see progress.
- We will turn the core/server rule on sooner than the core/shared one.
- Commented the core/shared rule out and put it first so I don't have to keep adding/removing the comma
refs: 10d02e8343
refs: 83a30775bf
refs: 3355f627c4
refs: 0e9b950558
- In eslint-plugin-ghost 2.4.0 I moved some of the rules implmented in Ghost as they are global rules, including:
- no new Error() - use @tryghost/errors
- forcing index.js to be under 50 chars as a leading indicator for misuse of index files
- I also added new rules to check for tpl('literal string') and the deprecated ghost-ignition package
- Because the new Error and tpl rules are both implemented with no-restricted-syntax, the local rules overrode the global one
- Removing the rule here allows the global ones to work
- Have to think about how to do this long term
- we have our own error library that should always be used to wrap errors in useful info
- therefore instead of new Error() we should always be using errors.SomeError from @tryghost/error
- We should not require server or frontend files inside the shared libraries as these are intended to be required FROM the server and frontend components
- Now that we've resolved the two outstanding warnings, make this an error so we can't regress on this easily
- index.js files are meant to be an index, not contain behaviour or logic
- files longer than 50 lines are indicative of code in the wrong place, all though not definitive
- enabling this as a hint to get us to move code to better locations
- We want to keep behaviour in services and libraries, not in API endpoints
- This rule runs complexity _only_ on the query methods, and has it set super low - just 3
- Methods that have higher complexity are a great indicator of places where we've left behaviour in the API, however!
- It's indicative, not definitive. At least with an eslint rule we can if needs be disable it where we decide the code is OK
- These rules will help us to enforce that server code should not be required from the frontend, and vice versa
- They are disabled/off for now because they are too noisy and not quick to fix
- Having them in place makes it easy to set them to warn to preview how we're getting on with fixing them ahead of enabling them
refs: https://github.com/TryGhost/Ghost/commit/7bce05ab8
- I wrote a custom plugin for the no-cross-requires logic between our modules after not finding anything that could do it
- Then, when searching for the next rule I wanted, I found eslint-plugin-ghost has no-restricted-requires
- This rule is more flexible, so switching to it
- NOTE: This update to eslint-plugin-ghost also fixes performance of linting our test files by pinning eslint-plugin-mocha to v7 as v8 has performance problems
- Using JS files to configure eslint gives us more power, e.g. being able to calculate paths
- We already use JS in pretty much every other repo we own, including admin... it's just Ghost we don't, and it's time!
