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

Partially migrating monorepo linting to microservices using turborepo #2805

Closed
wants to merge 1 commit into from

Conversation

tcvan0707
Copy link
Contributor

Issue This PR Addresses

Fixes #2736

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

This PR would be partially migrating our monorepo linting to our microservices using turborepo

Steps to test the PR

  • You should be in the root folder of Telescope
  • run pnpm turbo run lint

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Feb 2, 2022

@aserputov
Copy link
Contributor

@tcvan0707 why this is a draft PR?

@aserputov aserputov added dependencies Pull requests that update a dependency file type: enhancement New feature or request labels Feb 2, 2022
@aserputov aserputov linked an issue Feb 2, 2022 that may be closed by this pull request
@sirinoks
Copy link
Contributor

sirinoks commented Feb 2, 2022

@tcvan0707 why this is a draft PR?

This is work in progress

@tcvan0707
Copy link
Contributor Author

@tcvan0707 why this is a draft PR?

I need to get the reviews from reviewers first, I just added lint for src/web, not other parts yet. So I make this draft to get reviews

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

We need to update the way linting happens in CI to use turbo repo or something?

> @senecacdot/[email protected] eslint /home/runner/work/telescope/telescope
> eslint --config .eslintrc.js "**/*.{jsx,tsx,ts,js}"


Oops! Something went wrong! :(

ESLint: 7.32.0

ESLint couldn't determine the plugin "jest" uniquely.

- /home/runner/work/telescope/telescope/node_modules/.pnpm/[email protected]_cf0e947377590fbec5e3edece1a05d8c/node_modules/eslint-plugin-jest/lib/index.js (loaded in "--config")
- /home/runner/work/telescope/telescope/node_modules/.pnpm/[email protected]_9a712f10e802afc11b6eaaed0fb6f7e4/node_modules/eslint-plugin-jest/lib/index.js (loaded in "src/web/.eslintrc.js")

Please remove the "plugins" setting from either config or remove either plugin installation.

If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.

 ELIFECYCLE  Command failed with exit code 2.
Error: Process completed with exit code 1.

cc @menghif

overrides: [
// TypeScript for Next.js
{
files: ['src/web/**/*.ts', 'src/web/**/*.tsx'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to remove this bit from the root .eslintrc.js file, and only define the src/web stuff here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't it going through all the files with *.ts and *.tsx in src/web? I'm not sure what should be removed in here

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no src/web inside src/web, which is where you are right now.

The root .eslintrc.js needs to not also double-add this override (i.e., you are doing it here).

@menghif
Copy link
Contributor

menghif commented Feb 2, 2022

We need to update the way linting happens in CI to use turbo repo or something?
cc @menghif

Yes, I think we should use pnpm turbo run lint in CI.

I was helping @tcvan0707 with this PR and asked him to post it as a draft so we could all see it and contribute.

@menghif
Copy link
Contributor

menghif commented Feb 2, 2022

~/repos/telescope (issue-2736) % pnpm turbo run lint
• Packages in scope: @senecacdot/autodeployment, @senecacdot/feed-discovery-service, @senecacdot/image-service, @senecacdot/planet-service, @senecacdot/posts-service, @senecacdot/search-service, @senecacdot/sso-service, @senecacdot/status-service, @senecacdot/telescope-frontend, migrate, parser, users-microservice
• Running lint in 12 packages
@senecacdot/telescope-frontend:lint: cache miss, executing 83b7a9622a13c1cf
@senecacdot/telescope-frontend:lint: 
@senecacdot/telescope-frontend:lint: > @senecacdot/[email protected] lint /Users/francesco/repos/telescope/src/web
@senecacdot/telescope-frontend:lint: > pnpm eslint
@senecacdot/telescope-frontend:lint: 

 Tasks:    1 successful, 1 total
Cached:    0 cached, 1 total
  Time:    1.401s 

When running this, Turborepo looks inside all the packages defined in pnpm-workspace.yaml and runs the lint command. The problem is that it does not run lint in the root of the project.

Can we get away from having to run lint in the root all together once #2777 is done?

@humphd
Copy link
Contributor

humphd commented Feb 2, 2022

Yes, we can abandon the root directory and only do things in sub-projects.

@humphd
Copy link
Contributor

humphd commented Feb 8, 2022

@tcvan0707 how is this coming?

@humphd
Copy link
Contributor

humphd commented Feb 9, 2022

There's another way we could approach this, if it's going to be hard to get this resolution to work: similar to how we work with Satellite now, we could publish the Telescope eslint config to npm, and have pnpm include it as a versioned module from the registry. This would also solve the Dockerfile problem.

@menghif
Copy link
Contributor

menghif commented Feb 10, 2022

There's another way we could approach this, if it's going to be hard to get this resolution to work: similar to how we work with Satellite now, we could publish the Telescope eslint config to npm, and have pnpm include it as a versioned module from the registry. This would also solve the Dockerfile problem.

@tcvan0707 and I are going to implement this in PR: #2862

Closing this one.

@menghif menghif closed this Feb 10, 2022
@cindyorangis cindyorangis removed this from the 2.7 Release milestone Feb 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file type: enhancement New feature or request
Projects
None yet
6 participants