Skip to content

Fix syntax error in TypeScript when shape filenames contain hyphens #122

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.

Sign up for GitHub

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

Open
Copilot wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix syntax error in TypeScript when shape filenames contain hyphens #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1db9948
Initial plan
Copilot Jan 2, 2026
ba830f4
Fix syntax error when filenames contain hyphens by using valid TypeSc…
Copilot Jan 2, 2026
9044922
Fix code review issues: handle edge cases in toValidTypescriptIdentifier
Copilot Jan 2, 2026
99b55f9
Replace custom toValidTypescriptIdentifier with to-valid-identifier n…
Copilot Jan 3, 2026
d97cb64
Add engines field to specify Node.js >=20 requirement
Copilot Jan 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions packages/cli/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
"description": "A Command Line Interface for Linked Data Objects",
"main": "./dist/index.js",
"type": "module",
"engines": {
"node": ">=20"
},
"bin": {
"ldo": "./dist/index.js"
},
Expand Down Expand Up @@ -53,6 +56,7 @@
"prompts": "^2.4.2",
"rdf-dereference-store": "^1.4.0",
"rdf-namespaces": "^1.13.1",
"to-valid-identifier": "^1.0.0",
Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

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

The to-valid-identifier package requires Node.js >=20, but this package.json doesn't specify an engines field to enforce this requirement. Consider adding an engines field to prevent installation issues on older Node.js versions. For example:

"engines": {
  "node": ">=20"
}

This would make the requirement explicit and prevent users from encountering runtime errors when using Node.js versions below 20.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator

@jeswr jeswr Jan 3, 2026

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown
Collaborator

@jeswr jeswr Jan 3, 2026

Choose a reason for hiding this comment

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

Node that Node 18 is now EOL'd (https://nodejs.org/en/about/previous-releases)

Copy link
Copy Markdown
Author

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

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

Added engines field with "node": ">=20" in d97cb64.

"ts-morph": "^24.0.0",
"type-fest": "^2.19.0"
},
Expand Down
4 changes: 4 additions & 0 deletions packages/cli/src/build.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import loading from "loading-cli";
import { dirname } from "node:path";
import { fileURLToPath } from "node:url";
import { forAllShapes } from "./util/forAllShapes.js";
import toValidIdentifier from "to-valid-identifier";
Copy link
Copy Markdown
Contributor

@jaxoncreed jaxoncreed Mar 7, 2026

Choose a reason for hiding this comment

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

I'm not a fan of using this library to convert the identifier.

Here are some examples from its readme:

import toValidIdentifier from 'to-valid-identifier';

toValidIdentifier('foo');
//=> 'foo'

toValidIdentifier('foo-bar');
//=> 'foo$j$bar'

toValidIdentifier('$');
//=> '$a$'

toValidIdentifier('undefined');
//=> '$_undefined$'

While "$" is a valid javascript variable name, it seems clunky. I'd rather have hyphens simply removed from the variable name.

Copy link
Copy Markdown
Collaborator

@jeswr jeswr Mar 8, 2026

Choose a reason for hiding this comment

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

I would recommend against removing hyphens entirely, as this risks name conflicts (e.g. if we have shapes and _shapes) in the same directory.


// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
Expand Down Expand Up @@ -46,6 +47,8 @@ export async function build(options: BuildOptions) {
}
// Convert the content to types
const [typings, context] = await schemaConverterShex(schema);
// Convert fileName to a valid TypeScript identifier for use in generated code
const identifierName = toValidIdentifier(fileName);
await Promise.all(
["context", "schema", "shapeTypes", "typings"].map(
async (templateName) => {
Expand All @@ -54,6 +57,7 @@ export async function build(options: BuildOptions) {
{
typings: typings.typings,
fileName,
identifierName,
schema: JSON.stringify(schema, null, 2),
context: JSON.stringify(context, null, 2),
},
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/templates/context.ejs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { LdoJsonldContext } from "@ldo/ldo";

/**
* =============================================================================
* <%- fileName %>Context: JSONLD Context for <%- fileName %>
* <%- identifierName %>Context: JSONLD Context for <%- fileName %>
* =============================================================================
*/
export const <%- fileName %>Context: LdoJsonldContext = <%- context %>;
export const <%- identifierName %>Context: LdoJsonldContext = <%- context %>;
4 changes: 2 additions & 2 deletions packages/cli/src/templates/schema.ejs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Schema } from "shexj";

/**
* =============================================================================
* <%- fileName %>Schema: ShexJ Schema for <%- fileName %>
* <%- identifierName %>Schema: ShexJ Schema for <%- fileName %>
* =============================================================================
*/
export const <%- fileName %>Schema: Schema = <%- schema %>;
export const <%- identifierName %>Schema: Schema = <%- schema %>;
8 changes: 4 additions & 4 deletions packages/cli/src/templates/shapeTypes.ejs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { ShapeType } from "@ldo/ldo";
import { <%- fileName %>Schema } from "./<%- fileName %>.schema";
import { <%- fileName %>Context } from "./<%- fileName %>.context";
import { <%- identifierName %>Schema } from "./<%- fileName %>.schema";
import { <%- identifierName %>Context } from "./<%- fileName %>.context";
import {
<% typings.forEach((typing) => { -%>
<%- typing.dts.name %>,
Expand All @@ -17,8 +17,8 @@ import {
* <%- typing.dts.name %> ShapeType
*/
export const <%- typing.dts.name %>ShapeType: ShapeType<<%- typing.dts.name %>> = {
schema: <%- fileName %>Schema,
schema: <%- identifierName %>Schema,
shape: "<%- typing.dts.shapeId %>",
context: <%- fileName %>Context,
context: <%- identifierName %>Context,
};
<% }); -%>