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

Common issues caught in PR review #1479

Open
jattasNI opened this issue Sep 1, 2023 · 13 comments
Open

Common issues caught in PR review #1479

jattasNI opened this issue Sep 1, 2023 · 13 comments

Comments

@jattasNI
Copy link
Contributor

jattasNI commented Sep 1, 2023

🧹 Tech Debt

We'll use this issue to collect common issues found in PR review. Put the issues in comments, feel free to edit other people's comments with more info (does everyone have that permission?)

Once we've collected enough we can figure out what to do with the list. Maybe a code review checklist, maybe training material, maybe automated lint rules to catch this stuff for us, maybe capturing it in CONTRIBUTING in more detail.

@jattasNI jattasNI added tech debt triage New issue that needs to be reviewed labels Sep 1, 2023
@jattasNI
Copy link
Contributor Author

jattasNI commented Sep 1, 2023

Beachball change files

  1. wrong change type
  2. description is vague or focuses on implementation rather than client impact
  3. use the GitHub obfuscated email
  4. bump of monorepo package that is a devDep should use "dependentChangeType": "none" to prevent unnecessary new release of packages using it

@jattasNI
Copy link
Contributor Author

jattasNI commented Sep 1, 2023

Testing

  1. Page objects should not expose Element types as they leak implementation detail and expose too much API surface area. Instead they should expose methods that access specific properties of the element and return primitive types.
  2. All functionality should be covered by auto tests
  3. Tests should be at the appropriate level (unit test or Chromatic test)
  4. If something's not covered by auto tests, the PR description should include justification

@jattasNI
Copy link
Contributor Author

jattasNI commented Sep 1, 2023

Storybook

  1. Everything in the public API that we expect clients to use should be documented, including important events and methods. Documentation should follow the patterns of similar APIs on other components.
  2. No new accessibility violations were introduced

@jattasNI
Copy link
Contributor Author

jattasNI commented Sep 1, 2023

Styles

  1. Styles should be ordered according to our CSS conventions
  2. Don't set size attributes to inherit
    See examples in discussion 1 and discussion 2
  3. For sizing, use appropriate size tokens for the right context (i.e. control height), but for sizes without a context aligned token, use a size from the 4px grid size ramp.
    • Do not hard code specific pixel values, use the size ramp
    • If you need to add additional values to the size ramp, see the latest naming guidance decision
    • Yes the naming system for the fixed pixel size ramp is not great and uses the term "padding". It should still be used.
  4. The unset and revert keywords probably don't do what you think they do. Read the mdn docs carefully; they are probably not part of the solution you are looking for.
  5. Be aware of the display modes being applied to a elements in a template. Largely we should be using one of the layouts flex/grid (block) or inline-flex/inline-grid (inline). If there are a lot of different display modes being used to layout a template that may be a sign to double check the layout at a high-level.

@jattasNI
Copy link
Contributor Author

jattasNI commented Sep 1, 2023

Component index.ts

  1. Don't write logic in TypeScript if it can be expressed in the template or style instead. e.g. no size or style calculations in TS. No DOM manipulation in TS.

@m-akinc m-akinc removed the triage New issue that needs to be reviewed label Sep 5, 2023
@jattasNI
Copy link
Contributor Author

jattasNI commented Sep 5, 2023

PR structure

  1. PRs should do one thing and should not be too large
  2. PR title and description should be clear and detailed

@mollykreis
Copy link
Contributor

TypeScript

  1. Disabling lint rules should be rare and have good justification
  2. Use of non-null assertions (i.e. !) should be specific to places where TypeScript could not infer the type correctly. It should not be used to avoid properly handling corner cases in the code.

@rajsite
Copy link
Member

rajsite commented Mar 7, 2024

Buddy Reviews

The Nimble Code Review process states that contributions from outside the Nimble dev team should have reviews from a member of the Nimble Team before moving to Owners review.

If you are outside the NI organization, create an issue for discussion ideally before creating a PR.

If you are inside the NI organization and new to contributing to Nimble reach out on the Design System Teams channel to discuss your goals and get a Nimble point of contact.

Current Nimble Contacts:

@rajsite
Copy link
Member

rajsite commented Apr 19, 2024

Component Templates

  1. Don't use nullish coalescing in bindings in templates (other kinds of conditional logic should behave well).
    However, don't use any conditional logic in aggregate bindings (i.e. like when class is stitched together with multiple bindings).
    See detailed examples: Evaluate volatile template bindings #1843 (comment)

@rajsite
Copy link
Member

rajsite commented Jun 26, 2024

Related Changes

  1. We strive for consistency across components. When making a change in one spot, make sure to consider how that relates to out other components.
  2. We have three listbox / dropdown implementations (as of writing), select, combobox, and user mention listbox. A change in one should be considered against all three.

@rajsite
Copy link
Member

rajsite commented Jul 18, 2024

Upgrading dependencies

In the course of updating dependencies you may find that some dependencies get upgraded with a semver range that actually breaks the build. For example:

  • We have a dependency on "awesome-dep": "^1.0.0"
  • Our package-lock.json has a resolved dependency on [email protected] which is great, a patch release that does not break an API and fixes a bug.
  • An unrelated package is updated, etc, that incidentally updates the package-lock.json so it has a resolved dependency on [email protected] which is still in our declared semver range and would hopefully be great and fix a bug but, ah!, the package maintainers accidentally released a version that broke our build 😢
  • Do not try and tweak the package-lock.json by doing different incantations to avoid that package updating and get a passing build. You have found that our supported semver range is actually broken. The repo is in a bad state. We don't actually support awesome-dep@^1.0.0 because some of those patch or minor releases ended up being breaking.
    • What does it mean for the repo to be in a bad state? What is happening is that what we declare as a supported semver range, "awesome-dep": "^1.0.0", is not actually supported. It is particularly bad if it is a dependency instead of a devDependency. It means that potentially a new app that is created or a down stream client that rebuilds their lock file etc, will hit an issue leveraging our package because our own declared "supported" dependency range breaks us. It's unearthing a ticking time bomb that will blow. Something to address ASAP.
  • Do feel a little sad that you are temporarily blocked and try to resolve the issue. It may be package-specific but some options:
    • Rebuild the lock file from scratch to make sure everything is up-to-date by locally deleting the package-lock.json, doing a git clean -fdx to purge all node_modules, and running npm install to create a new lock file.
    • If the new clean lock file is still failing the build you may need package.json updates to resolve to supported versions. Some options:
      • fix the underlying issue and update the semver supported range to the new minimum that is supported, i.e. "awesome-dep": "^1.0.2", rebuild the lock file, and submit
      • pin a dependency, i.e. "awesome-dep": "1.0.1" (the last good known version), rebuild the lock file and see if the new resolved version fixes the issue, and create a tech debt item to unpin the version (and make sure it moves towards being fixed!)

@rajsite rajsite mentioned this issue Jul 18, 2024
1 task
@rajsite
Copy link
Member

rajsite commented Sep 10, 2024

Handling unstable stories

If you see a chromatic diff for unstable stories (such as the menu stories) verify the snapshots but do not accept bad snapshots. Instead to create good snapshots you should be able to do the following:

  • Bump the GUID in preview.js to rebuild all snapshots.
  • Verify that all snapshots are being taken (i.e. zero turbo snaps)
  • Use the rebuild command to see if the stories settle:
    image
  • Note: You may need 1-2 cycles of modifying the token in preview.js and using the rebuild command

@rajsite
Copy link
Member

rajsite commented Sep 11, 2024

Avoid memory leaks

  • Make sure event registrations, i.e. addEventListener and removeEventListener are cleaned up in the disconnectedCallback
  • Make sure notifier subscriptions, i.e. Observable.getNotifier and notifierRef.subscribe are cleaned up in the disconnectedCallback

rajsite added a commit that referenced this issue Dec 4, 2024
# Pull Request

## 🤨 Rationale

Updates dependencies manually to address build issues revealed by #2482
#2481 #2480.
Found that just updating dependencies by rebuilding the lock fails the
build which means the [repo is in a bad
state](#1479 (comment))
and that should be addressed quickly.

## 👩‍💻 Implementation

- Update major deps and rebuild lock
- Found that the now [deprecated tiptap link
validate](https://tiptap.dev/docs/editor/extensions/marks/link#validate-deprecated)
function is expected to still function with [backwards
compat](ueberdosis/tiptap#5812) however there
seems to be a regression as it [fails our build by allowing autolinking
of unexpected
protocols](https://github.com/ni/nimble/actions/runs/12110590706/job/33761233944#step:17:613).
- Switched from tiptap link validate to
[shouldAutoLink](https://tiptap.dev/docs/editor/extensions/marks/link#shouldautolink)
as described by docs. That seems to not fail the build.
- Pushed the min semver range version up for all the tiptap /
prosemirror packages as otherwise it seems to[ fail build when type
checking libraries](ueberdosis/tiptap#5867).
   
**NOTE**: It seems to be transitive dependency type issues that will
fail builds, I think it's possible that minimal upgrades done by
renovate may hit issues in app updates. If an app adopting latest these
changes fails to build it may need to rebuild the lock files in a clean
workspace to align transitive deps on latest versions and avoid library
type check issues.
- Updates to [prettier
3.4](https://prettier.io/blog/2024/11/26/3.4.0.html) resulting in lots
of changes.

## 🧪 Testing

Rely on CI.

## ✅ Checklist

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

No branches or pull requests

4 participants