Skip to content

Add Node example validator - #253

Open
Aryex wants to merge 17 commits into
mainfrom
alexl/node-ex-validator
Open

Add Node example validator#253
Aryex wants to merge 17 commits into
mainfrom
alexl/node-ex-validator

Conversation

@Aryex

@Aryex Aryex commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR implement the Node example validator.

Issue link

#237

Content Changes

  • Added a new scripts/validators/node.py to validator Node examples.
  • Updated workflow to check C# and Node examples in a single workflow.
  • Fixed Node code examples found.
  • Moved csharp validator script into validator/ folder.

Implementation

Follows the implementation per #237.

@Aryex
Aryex force-pushed the alexl/node-ex-validator branch 3 times, most recently from fce337b to f5069c5 Compare June 22, 2026 16:40
@Aryex
Aryex marked this pull request as ready for review June 22, 2026 21:26
@Aryex
Aryex requested a review from currantw June 22, 2026 21:26
@Aryex
Aryex force-pushed the alexl/node-ex-validator branch 2 times, most recently from db2cc54 to ea8c3c6 Compare June 22, 2026 21:46
@Aryex Aryex changed the title Implemented Node example validator Add Node example validator Jun 22, 2026
@currantw currantw added documentation Improvements or additions to documentation nodejs ci/cd labels Jun 23, 2026

@currantw currantw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed to ea8c3c6.


// Retrieve statistics
const stats = await client.getStatistics();
const stats = await client.getStatistics() as Record<string, any>;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmmm. Why is this cast necessary? Are we not properly specifying the return type for getStatistics? 🤔

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

getStatistics return type is an Object so the example need to cast it to use the . notation.

on:
pull_request:
branches: [main]
workflow_dispatch:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

workflow_dispatch isn't included in the corresponding C# workflow. Should we add it for consistency?

runs-on: ubuntu-latest
steps:
- name: Checkout valkey-glide-docs
uses: actions/checkout@v4

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One of the security findings for the C# repo was that we should use commit hashes rather than version numbers, e.g.

uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5

(As I understand it, a commit hash cannot be changed, but a malicious actor who was able to gain control of a repo could update the tag to point to a new commit and so download different code).

Are you able to raise an issue for this on the docs repo? Should be a very easy issue to fix with AI.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Created #261

Comment on lines +22 to +26
- name: Checkout valkey-glide
uses: actions/checkout@v4
with:
repository: valkey-io/valkey-glide
path: valkey-glide

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would there be any advantages to doing a limited checkout of the few directories (e.g. node/, ffi/, and glide-core) that are actually needed for this job?

(The C# validator workflow doesn't checkout the valkey-glide submodule for same reason!)

@Aryex Aryex Jun 23, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We could restrict it but I don't think it would net us much improvements.

The main bottleneck currently is during the Node client build phase. It has to build the Rust layer and Node, essentially building the full client. The C# checker only has to build the C# layer so it doesn't need valkey-glide submodule, and is a lot quicker.

Comment on lines +48 to +52
- name: Build Node client
working-directory: valkey-glide/node
run: |
npm ci
npm run build:release

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In the C# example, we are able to skip building the Rust core because it actually isn't needed to validate the examples syntax. Are there any similar optimizations that we could perform here? I know that building the Rust core can be quite slow. What about installing all of the node modules?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yup this is currently the biggest bottleneck here. Updating this to match C# would require a code change on Node side although I haven't looked at the scope of this.

Comment on lines +42 to +44
def _extract_examples() -> dict[str, str]:
"""Extract TypeScript/JS code blocks from all MDX files."""
examples: dict[str, str] = {}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Some more documentation of the return type (similar to what exists in the current C# script) might be useful, I think.

@Aryex Aryex Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Moved to _common.py and updated docstring.

See 244a629

Comment on lines +72 to +82
_DEFAULT_IMPORTS = (
"GlideClient, GlideClusterClient, GlideClientConfiguration, "
"GlideClusterClientConfiguration, Batch, ClusterBatch, BatchOptions, "
"ClusterBatchOptions, ClusterBatchRetryStrategy, Script, Transaction, "
"Routes, Logger, RequestError, ServerCredentials, GlideFt, "
"GlideJson, OpenTelemetry, ClientSideCache, EvictionPolicy, TimeUnit, "
"Field, FtSearchOptions, FtSearchReturnType, Decoder, "
"AdvancedGlideClusterClientConfiguration, CompressionBackend, "
"OpenTelemetryConfig, OpenTelemetryMetricsConfig, OpenTelemetryTracesConfig, "
"ClusterScanCursor, ReadFrom, GlideString, PubSubMsg, ALL_CHANNELS, ALL_PATTERNS"
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why are these grouped into multi-entry strings like this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It is a formatted form from a single line import.

line = lines[i]
if _IMPORT_LINE.match(line):
import_lines = [line]
while not re.search(r"""from\s+["'][^"']+["']\s*;?\s*$""", line) and i + 1 < len(lines):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Perhaps we should extract this regex? Otherwise, I think it might re-compile on each pass through the loop (not sure whether a typical Python interpreter would optimize this?)

@Aryex Aryex Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Extracted. See 0761d6a

Comment on lines +119 to +129
for imp in imports:
m = re.search(r"\{([^}]+)\}", imp)
if m:
for name in m.group(1).split(","):
existing_names.add(name.strip())
defaults = [
n.strip() for n in _DEFAULT_IMPORTS.split(",")
if n.strip() not in existing_names
]
if defaults:
parts.append(f"import {{ {', '.join(defaults)} }} from \"@valkey/valkey-glide\";\n")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's not completely clear to me how the logic in here works, or what the re.search call is doing. Maybe some comments would be useful?

@Aryex Aryex Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Refactored in 8a7a4fb.

We are trying to build the import statement, but have to take care and remove duplicated imports as it counts as errors for tsc.

Comment on lines +259 to +260
if source:
result[source] = msgs

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Under what circumstances would this not be part of the dict?

@Aryex Aryex Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Removed. See 7ac52e6

Aryex added 4 commits June 29, 2026 09:24
Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
Aryex added 9 commits July 1, 2026 17:06
Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
@Aryex
Aryex force-pushed the alexl/node-ex-validator branch from ea8c3c6 to 7ac52e6 Compare July 2, 2026 00:07
Signed-off-by: Alex Le <alex.le@improving.com>
Aryex added 3 commits July 1, 2026 17:58
Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/cd documentation Improvements or additions to documentation nodejs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants