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

build: make sure eslint-plugin-calcite-components is set up correctly #7294

Merged
merged 6 commits into from
Jul 7, 2023

Conversation

benelan
Copy link
Member

@benelan benelan commented Jul 7, 2023

Related Issue: #

Summary

I forgot to mention that the packages within the monorepo are "local dependencies", which means npm install won't install them, they have to be built locally. A common workflow is cding directly into calcite-components to do work. This caused issues with eslint-plugin-calcite-components because it wasn't being built so it wasn't working during development in CC.

I added a start turbo pipeline which will ensure all dependencies are built first. So running npm start from the root directory will no longer cause this issue.

I also cleaned up the Contributing doc and tweaked some eslint settings that were causing issues in VSCode.

@benelan benelan requested a review from a team as a code owner July 7, 2023 01:55
@github-actions github-actions bot added the chore Issues with changes that don't modify src or test files. label Jul 7, 2023
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome 👏

@benelan
Copy link
Member Author

benelan commented Jul 7, 2023

@jcfranco I had to remove the on-exit cleanup of js demo files from npm start because it was breaking the turbo pipeline. I'm guessing turbo's concurrency was getting tangled up with concurrently and the exit code watcher.

Instead, I clean out all of the js files created by tsc before running npm start and make sure the files that were previously being cleaned are gitignored.

Is that solution okay with you or should I spend more time troubleshooting the concurrency issue?

CONTRIBUTING.md Outdated
@@ -112,11 +112,11 @@ If your IDE supports the [Language Server Protocol (LSP) specification](https://

## Starting the demos

First, clone the repo and install the NPM dependencies from within the `calcite-components` directory:
First, clone the repo, install the NPM dependencies:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I feel like we need an "and then" between cloning and installing deps. I'll leave this up to you.

@@ -0,0 +1,3 @@
{
"ignorePatterns": ["*"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any package with this configuration will skip linting entirely, right? If so, won't this make the contributing instructions on linting misleading?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, maybe the contributing doc can be updated to indicate that ESLint will only run on calcite-components and we can tweak our linting config to lint properly outside calcite-components after this lands?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I added this to CCR and the eslint plugin so eslint won't complain about there not being a config which covers their files.

I'm working on pulling shared eslint/tsconfigs into separate workspaces as part of the lint-staged fix. After that happens, the base eslint config will be used in these packages.

So I'm going to leave the doc as is for now so I don't have to change it and then change it back.

Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from a comment regarding linting on other packages, this LGTM! 🚀

@@ -0,0 +1,3 @@
{
"ignorePatterns": ["*"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, maybe the contributing doc can be updated to indicate that ESLint will only run on calcite-components and we can tweak our linting config to lint properly outside calcite-components after this lands?

@benelan
Copy link
Member Author

benelan commented Jul 7, 2023

Note: I couldn't get the tsconfig/eslint separating to work at the moment so I updated the doc. See #7122 (comment)

@benelan benelan merged commit 89349d6 into main Jul 7, 2023
@benelan benelan deleted the benelan/eslint-plugin-build-required branch July 7, 2023 22:30
@github-actions github-actions bot added this to the 2023 July Priorities milestone Jul 7, 2023
benelan added a commit that referenced this pull request Jul 12, 2023
* origin/main:
  chore: release next
  fix(block): loader now appears for all loading cases (#7303)
  build: update browserslist db (#7301)
  build(deps): Bump eslint-plugin-jsdoc from 46.2.6 to 46.4.3 (#7306)
  build(deps): Bump @floating-ui/dom from 1.4.3 to 1.4.4 in /packages/calcite-components (#7305)
  build(deps): Bump chromatic from 6.19.5 to 6.19.9 (#7308)
  build(deps): Bump cpy-cli from 4.2.0 to 5.0.0 (#7281)
  chore: release next
  feat(text-area): provide additional context for AT users when character limit exceeds (#7299)
  build(deps): Bump stylelint from 14.16.1 to 15.10.1 (#7296)
  chore: release next
  ci: allow lint-staged to find typescript parser config file (#7297)
  fix(accordion, accordion-item): `icon-position`, `icon-type`, `selection-mode` and `scale` can now be set as props or attributes (#7191)
  test(stack): stabilize screenshot tests (#7298)
  build: make sure eslint-plugin-calcite-components is set up correctly (#7294)
  chore: add translations (#7290)
  refactor: Ensure all setFocus methods return calls to setFocus (#7287)
  chore: release next
  fix(card): ensure teardown logic is called when disconnected (#7289)
  test: stabilize tests using setFocus (#7295)
jcfranco added a commit that referenced this pull request Jul 19, 2023
**Related Issue:** N/A

## Summary

Update doc scripts to no longer reference `cleanOnProcessExit.ts`
(removed via #7294).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Issues with changes that don't modify src or test files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants