Skip to content

fix(librarian/nodejs): run eslint directly for formatting#4592

Merged
julieqiu merged 5 commits intogoogleapis:mainfrom
julieqiu:nodejs-05-eslint-format
Mar 15, 2026
Merged

fix(librarian/nodejs): run eslint directly for formatting#4592
julieqiu merged 5 commits intogoogleapis:mainfrom
julieqiu:nodejs-05-eslint-format

Conversation

@julieqiu
Copy link
Copy Markdown
Member

@julieqiu julieqiu commented Mar 15, 2026

Format is updated to run eslint directly instead of npm run fix (gts).

The previous approach of running npm run fix failed because gts expands file globs (like src/**/*.ts) at the shell level. This caused it to pick up and attempt to lint files within the node_modules directory, leading to formatting failures. Running eslint directly with explicit --ignore-pattern node_modules/ ensures that only the generated source files are processed.

Additionally, this switch allows us to use a pre-installed global eslint, avoiding runtime network dependencies and the need for a local npm install during code generation. We use eslint@8 to maintain compatibility with the legacy .eslintrc.json format still used across existing libraries. See #4590 for additional context.

For #4404

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the Node.js formatting to use eslint directly, which is a good improvement to remove network dependencies and fix globbing issues. The implementation is solid, and the accompanying test is thorough. I have a couple of minor suggestions to improve code style and align with the repository's Go guidelines, mainly around error handling patterns in both the implementation and the test code.

Comment thread internal/librarian/nodejs/generate.go Outdated
Comment thread internal/librarian/nodejs/generate_test.go Outdated
The Format function is updated to run eslint directly instead of npm run fix (gts).

The previous approach of running npm run fix failed because gts expands file globs (like src/**/*.ts) at the shell level. This caused it to pick up and attempt to lint files within the node_modules directory, leading to performance issues and formatting failures. Running eslint directly with explicit --ignore-pattern node_modules/ ensures that only the generated source files are processed.

Additionally, this switch allows us to use a pre-installed global eslint, avoiding runtime network dependencies and the need for a local npm install during code generation. We use eslint@8 to maintain compatibility with the legacy .eslintrc.json format still used across existing libraries.

Fixes #116
@julieqiu julieqiu force-pushed the nodejs-05-eslint-format branch from 7eee56d to 39c2b84 Compare March 15, 2026 14:55
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 15, 2026

Codecov Report

❌ Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.27%. Comparing base (1eadeb4) to head (22a4eeb).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
internal/librarian/nodejs/generate.go 0.00% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4592      +/-   ##
==========================================
- Coverage   79.39%   79.27%   -0.13%     
==========================================
  Files         110      110              
  Lines        9223     9235      +12     
==========================================
- Hits         7323     7321       -2     
- Misses       1389     1403      +14     
  Partials      511      511              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gapic-generator-typescript takes over 4 minutes to install.
Increased timeout to 6 minutes until performance is improved.
Updated TODO comment with a link to the issue for gapic-generator-typescript installation time.
@julieqiu julieqiu marked this pull request as ready for review March 15, 2026 15:17
@julieqiu julieqiu requested a review from a team as a code owner March 15, 2026 15:17
@julieqiu julieqiu enabled auto-merge (squash) March 15, 2026 15:17
@julieqiu julieqiu merged commit b5b369b into googleapis:main Mar 15, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants