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

Update linters for VS Code, move files from kobo-common into kpi #4985

Merged
merged 4 commits into from
Jun 27, 2024

Conversation

p2edwards
Copy link
Contributor

@p2edwards p2edwards commented Jun 25, 2024

Description

Update linter dependencies to support VS Code. Move colors.scss and linter definitions from kobotoolbox/kobo-common directly into kobotoolbox/kpi to make updating and development easier.

Notes

This PR fixes our linters so they work with current versions of VS Code extensions (eslint, Stylelint, Prettier), and changes the way these linters are loaded to make future updates easier.

  • Upgrade each linter dependency to the latest version that still supports Node 16.
    • These are due for upgrade again soon when we move Node to 18, 20, or 22.
  • Move linter configs from kobo-common into kpi
    • This makes them easier to maintain,
    • more compatible with VS Code tooling,
    • and easier to review in relation to kpi PR's.
  • Move colors.scss from kobo-common into kpi.
    • Similar advantages for PR's and maintainability,
    • plus this lets us remove kobo-common from package.json, streamlining the update process.

Future plans for the linters include updating to ESLint 'flat config', separating 'stylistic' and 'inference' rules, and adding some library-specific linters.

Re-adopting these scripts in the kpi repo will make it easier to keep them working, up-to-date, and useful (congruent with our current practices). The kobo-common workflow was adding a little too much friction to common tasks, causing these files to get sidelined:

  • .eslintrc.js
  • .editorconfig
  • .prettierrc.js
  • .stylelintrc.js
  • colors.scss

Importing them via npm adds several extra steps every time we:

  • change any of them
  • update a shared npm package
    • or add a new linter-related package
    • need to sync versions in both
  • need to troubleshoot tools
    • tools that don't expect nested dependencies
    • npm dependency resolution
    • git hooks, cli paths, version tags
  • read or write PR's across two repos
    • authoring friction: write and manage commits in two repos, while running npm commands to keep them synced
    • reviewer friction: 1 PR becomes 2 PR's
    • checkout friction: more re-install's required between checkouts

One of the goals of kobo-common — to let other repos subscribe to common assets primarily created for kpi — can be replaced with a script, or a periodic semi-automated task, which doesn't need to happen very often. We can still accomplish 'DRY' by more primitive mechanisms, without paying the overhead of using npm + git hooks to factor these out.

Related issues

Part of #4703
Related to #4230

Checklist

  1. If you've added code that should be tested, add tests
  2. If you've changed APIs, update (or create!) the documentation
  3. Ensure the tests pass
  4. Make sure that your code lints and that you've followed our coding style
  5. Write a title and, if necessary, a description of your work suitable for publishing in our release notes
  6. Mention any related issues in this repository (as #ISSUE) and in other repositories (as kobotoolbox/other#ISSUE)
  7. Open an issue in the docs if there are UI/UX changes

- Linter packages are updated to the last versions supported before
    deprecating Node 16. (so, these will be updated again soon.)

- Extensions for VS Code are working again.
    (recent auto-updates dropped support for the versions we were on)

Moving these support files directly into the kpi repo makes it easier
to:

- edit the files -- less indirection; can include linter updates as part
    of regular commits during feature work)

- keep dependencies up to date -- there are ways to manage these npm
    packages between multiple repos in lockstep but it's a lot more
    complicated to do it like that

- support VS Code -- there's an unstated assumption among the standard
    extensions that your language- and build-related npm packages are
    installed at the top level of your projects. it's one more reason
    an extension might be broken at any given time. your options are:

    (1) install once, at the root level of the project, and rule out
        that this is a problem

    (2) install in your nested module and install a copy with a
        matching version at the root level, too, update them in lockstep
        and cross your fingers

    shout out to Sublime / Neovim for not having this problem
Show the prompt that lets VS Code run the local TypeScript version in
your workspace, so it matches the output of `npx tsc`.

---

The error in formGallery/utils.ts is weird. This StackOverflow snippet
checks if something is an instance of an Object or Array. TS2358 aptly
notices that the right-hand side of

   value instanceof Object || value instanceof Array

will never be reached, since an array is also an instanceof Object.

So this could become

   value instanceof Object

…but to preserve the meaning of the code, I've just switched their
order around.

---

Updated TS while I'm at it, since the code still builds without an issue
and it's nice to keep on top of it.
@p2edwards p2edwards added workflow Related to development process Front end labels Jun 26, 2024
@magicznyleszek magicznyleszek self-assigned this Jun 27, 2024
@p2edwards p2edwards merged commit f7569d9 into beta Jun 27, 2024
5 checks passed
@p2edwards p2edwards deleted the update-move-linters-jun-24 branch June 27, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Front end workflow Related to development process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants