chore: Deprecate Structure, introduce Skeleton

[Josh-Walker-GM]

WIP: Initial implementation, likely to change and develop significantly

Problem
The structure package was created with the Redood VSCode extension in mind and so is written in a style optimised for that use case. That use case no longer exists and the remaining introspection functionality which remains in use within the framework would benefit from an updated package which would be more maintainable and have a more convenient api.

Solution
Remove the structure package and implement a replacement package which provides the introspection ability, here it's named skeleton.

Description
The skeleton package implements a series of classes which model the individual components of a Redwood project; cells, routes etc. These components can be extracted from a project either individually or as part of a larger project tree. The properties of the components are determined by analysing the AST from the project files.

Functions are provided to extract the individual components. Taking cells as an example you may extract one cell from a file using extractCell or extract all the cells within a project using extractCells. This is true for the other components such as pages, routers etc. It is also possible to generate a full project using RedwoodProject.getProject. The goal is to provide flexibility.

Caching is introduced to prevent the need to repeatedly extract, generate and process the AST from the individual js/ts project files.

Changes

1. "skeleton" package added
2. Prerender package: uses skeleton for detecting prerendered routes
3. CLI package: uses skeleton for prerender command
4. CLI package: uses skeleton for check command
5. CLI package: uses skeleton for cell generator/destroy to detect cell graphql query names
6. Internal package: uses skeleton for the duplicate route detection
7. Internal package: uses skeleton for mock cell babel plugin
8. Telemetry package: uses skeleton for getting complexity metric and sides list

Outstanding

Whilst there is a large amount of possible outstanding additions the sensible decision is to prioritise only cells and routes as these are the components which currently require the full AST introspection.

4. Additional checks could be added to match those that exist in structure
5. @dac09 mentioned that incorporating https://github.com/fastify/fastify-dx/blob/main/URMA.md would potentially be nice
6. Much more...

Pings
@Tobbe, @dac09

[thedavidprice]

rw check command usage

Turns out this is a very popular command! Some overviews of peaks from past three months:

• Non-CI: between 500-700 weekly runs
• CI (excluding Redwood Framework CI): 2,500-4,500 weekly runs

Implications

• we should keep it around, for sure
• it's worth considering future opportunities to make it better... even if just starting an Issue with a "Roadmap Wishlist"

Graphs

Note: most recent data point includes an incomplete current week

CI (excluding Redwood Framework CI)

Non-CI

[Josh-Walker-GM]

Excellent to have this data to guide decisions!

I plan on going through structure and documenting all it's current diagnostic checks. I will implement them all within skeleton to ensure users still get all the same checks.

I have some ideas floating around about error/warning enumeration and how that could couple with docs around these errors/warnings which are checked for. Will probably need to get everyone's thoughts on that idea though.

It would be interesting if the check command could become a go to tool for developers. Perhaps running it in the background during development. Asking what the command output is in the issue template to gather more debug data etc. I can imagine it filling a niche left by deprecating the editor extension.

PR status update
A general update on the PR. Skeleton is nearing feature parity with structure. The following remains to be done:

1. Service and SDL extraction to support the telemetry package. Used to generate a measure of a projects complexity.
2. The check command output has less information. It doesn't show line numbers and does not include all of structures checks yet.

[thedavidprice]

I plan on going through structure and documenting all it's current diagnostic checks. I will implement them all within skeleton to ensure users still get all the same checks.
...
I have some ideas floating around about error/warning enumeration and how that could couple with docs around these errors/warnings which are checked for. Will probably need to get everyone's thoughts on that idea though.

Sounds great!

For this PR, check feature parity is perfect. Feel free to move additional / make-it-better items to a new Issue or PR.

I can imagine it filling a niche left by deprecating the editor extension.

^^ yes, exactly!!

[Josh-Walker-GM]

The check command tests which structure currently provides are, to the best of my knowledge, listed below.

#	Type	Check	Structure	Skeleton
1	Err	Every cell must have a "QUERY" export	✅	✅
2	Err	Every cell must have a "Success" export	✅	✅
3	Warn	GQL operation should have a name	✅	✅
4	Err	Cell syntax errors	✅	✅
5	Err	Route corresponding page not found	✅	✅
6	Err	Route invalid path syntax	✅	❔
7	Err	Route notfound cannot be private	✅	✅
8	Err	Route notfound cannot have a path	✅	✅
9	Err	Route only routes with parameters can have prerender info	✅	❌
10	Err	Router routes file does not exist	✅	✅
11	Err	Router only one notfound route	✅	✅
12	Err	Router must have a notfound route	✅	✅
13	Err	SDL file must export a schema variable	✅	✅
14	Err	SDL Field service not implemented	✅	✅
15	Err	TOML is valid syntax	✅	✅
16	Err	Service function parameter mismatch between SDL	✅	✅
17	Warn	Env value is not available	✅	❌
18	Warn	Env value is present but not in includeEnvironmentVariables	✅	❌

Some additional tests have been added because they seemed natural at the time:

#	Type	Check
1	Err	Router duplicate routes with the same name present
2	Err	SDL contains a directive which is not defined
3	Err	Directive should define one directive per file
4	Err	Directive is missing schema export
5	Err	Directive is missing a default export
6	Err	Function should have a handler export
7	Warn	Router has no routes

All the warnings and errors are enumerated in the src/components/diagnostic.ts file.

---

Note: the TOML check (15) is added but the whole skeleton package relies on getPaths which reads the toml config anyway so it fails before it can even check.

Note: I didn't include the prerender info check (9) because it was hardcoded to never been thrown as an error.

Note: I think in an invalid route syntax will throw an error with the check command itself because of the way getJSXElementAttributes in lib/ast.ts currently works.

Note: I didn't include the ENV var warning checks yet. I wanted to confirm we still want these checks. I think this would mean reading ALL project files and checking all the env var uses?

[Josh-Walker-GM]

Anyone any idea why this exists? I noticed it being flagged up as a parser error in skeleton because it's looking for a name for each export.

redwood/__fixtures__/test-project/api/src/services/users/users.ts

Line 5 in ac645b1

export {}

[jtoar]

@Josh-Walker-GM I just fixed merge conflicts for you—let me know if this was undesired and I'll revert. You can see the changes here: 539abb8.

[replay-io]

9 replays were recorded for c090dc5.

0 Failed

9 Passed

• useAuth hook, auth redirects checks
• Check that tags are rendering the correct dynamic data
• Check that a specific blog post is prerendered
• Check that about is prerendered
• Check that homepage is prerendered
• Check that you can navigate from home page to specific blog post
• RBAC: Admin user should be able to delete contacts
• RBAC: Should not be able to delete contact as non-admin user
• Smoke test with dev server

View test run on Replay ↗︎

[jtoar]

This is awesome @Josh-Walker-GM, if only for the sheer amount of code. We're going to have to have a lot of back and forths, and run a lot of experiments before it's even close to merging, but I think this has the potential to solve a lot of problems for us. I'm going to need to go through it at least a couple more times (I only made it through a few files), but I've got a lot of comments:

• there's lots of enums. the team (and TS community at large) is divided on them. just keep that in mind
• we should think even bigger here and try remove this package's dependency on @redwoodjs/internal entirely. probably by subsuming the functionality it's importing
• it's definitely worth seeing if we can play more into composition over inheritance, but I need to get a complete grasp of the code first. don't make any radical changes yet, unless you feel strongly

[jtoar]

When I started learning TS I was told to let TS infer the return type as much as possible. Not sure if things have changed, but unless you've heard otherwise let's remove the return type for functions like these:

Suggested change

	hasWarnings(): boolean {
	return this.warnings.length > 0
	}
	hasWarnings() {
	return this.warnings.length > 0
	}

[jtoar]

Maybe reuse hasWarnings here? I also prefer less nesting by returning early, but that's just my preference:

Suggested change

	printWarnings(): void {
	if (this.warnings.length > 0) {
	const titleLine = `${chalk.bgYellow('[Warn]')}\t${this.name} ${chalk.dim(
	this.filepath
	)}`
	const warningLines = this.warnings.map((warning) => {
	return ` (W${warning.code}) ${warning.message}\n`
	})
	console.log(titleLine.concat('\n', ...warningLines).trimEnd())
	}
	}
	printWarnings() {
	if (!this.hasWarnings()) {
	return
	}
	const titleLine = `${chalk.bgYellow('[Warn]')}\t${this.name} ${chalk.dim(
	this.filepath
	)}`
	const warningLines = this.warnings.map((warning) => {
	return ` (W${warning.code}) ${warning.message}\n`
	})
	console.log(titleLine.concat('\n', ...warningLines).trimEnd())
	}

[jtoar]

Suggested change

	printErrors(): void {
	if (this.errors.length > 0) {
	const titleLine = `${chalk.bgRed('[Error]')}\t${this.name} ${chalk.dim(
	this.filepath
	)}`
	const errorLines = this.errors.map((error) => {
	return ` (E${error.code}) ${error.message}\n`
	})
	console.log(titleLine.concat('\n', ...errorLines).trimEnd())
	}
	}
	printErrors() {
	if (!this.hasErrors()) {
	return
	}
	const titleLine = `${chalk.bgRed('[Error]')}\t${this.name} ${chalk.dim(
	this.filepath
	)}`
	const errorLines = this.errors.map((error) => {
	return ` (E${error.code}) ${error.message}\n`
	})
	console.log(titleLine.concat('\n', ...errorLines).trimEnd())
	}

[jtoar]

The handbook says public is redundant https://www.typescriptlang.org/docs/handbook/2/classes.html#public:

Suggested change

	public readonly name: string
	readonly name: string

[jtoar]

You're gonna have to walk me through this one. I'm mainly confused why (or how even) filepath is accessed off this in the constructor's body when it's passed as an arg?

[jtoar]

You can simplify this a lot:

Suggested change

	export function extractCells(
	project: RedwoodProject | undefined = undefined
	): RedwoodCell[] {
	export function extractCells(project?: RedwoodProject): RedwoodCell[] {

[jtoar]

You may be able to save on duplication here (and in the other get* methods) by using deocrators. Though you probably want to wait for TS v5 since it has a different (more accurate to modern JS) decorator implementation https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-beta/#decorators

[jtoar]

May as well use the class you're extending from:

Suggested change

	let warningsFound = this.warnings.length > 0
	let warningsFound = super.hasWarnings()

[jtoar]

Suggested change

	let errorsFound = this.errors.length > 0
	let errorsFound = super.hasErrors()

[jtoar]

It's probably worth using JS's private field syntax (#cell, and when accessing, this.#cell) unless you have a good reason otherwise since it isn't one to one with TS's private keyword: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_class_fields

Also, the ? and undefined union look redundant to me; I'd just go with ?

Suggested change

	private cells?: RedwoodCell[] | undefined
	private directives?: RedwoodDirective[] | undefined
	private functions?: RedwoodFunction[] | undefined
	private layouts?: RedwoodLayout[] | undefined
	private pages?: RedwoodPage[] | undefined
	private routers?: RedwoodRouter[] | undefined
	private sdls?: RedwoodSDL[] | undefined
	private services?: RedwoodService[] | undefined
	private sides?: RedwoodSide[] | undefined
	private tomls?: RedwoodTOML[] | undefined
	#cells?: RedwoodCell[]
	#directives?: RedwoodDirective[]
	#functions?: RedwoodFunction[]
	#layouts?: RedwoodLayout[]
	#pages?: RedwoodPage[]
	#routers?: RedwoodRouter[]
	#sdls?: RedwoodSDL[]
	#services?: RedwoodService[]
	#sides?: RedwoodSide[]
	#tomls?: RedwoodTOML[]

[Tobbe]

I'm sorry I'm late to the party here, but why not use TS's class syntax when in TS land?
private gets you: Nicer syntax for defining, nicer syntax for accessing, faster performance (meh, most likely negligible)
# gets you: Real privacy (can't do project['cells'])
So, yeah, technically/theoretically # is better. But practically I prefer private

[Josh-Walker-GM]

Thanks @jtoar a few comments:

there's lots of enums. the team (and TS community at large) is divided on them. just keep that in mind

I have seen that TS enums aren't the enums of my childhood and that they don't behave well under certain conditions. There's 4 enums in use. Error and warning enumeration - I'd like these enumerated but don't have to use TS enums for this if there are better approaches. The other two are side and is the project js or ts which on reflection can probably be easily changed to a union of strings.

Edit: I updated the side and js/ts enum to string unions.

we should think even bigger here and try remove this package's dependency on @redwoodjs/internal entirely. probably by subsuming the functionality it's importing

This is what #6919 was hoping to do? I was thinking that the paths package was going to be the lighter weight package which contains all the paths/config utilities. Then it would be the case we rely on that paths package not internal. It also means that we'd not be duplicating the path/config logic between paths and skeleton and that you wouldn't have to haul skeleton about if you wanted path/config functionality.

[dac09]

Hey @Josh-Walker-GM - Tobbe and I had a chance to go through this together, and here's what we came up with. Either of us will be happy to pair with you to get these changes in!

1. Could we move all the "introspection" capabilities in rwjs/internal to this package? Files/functions like:

   • packages/internal/src/ast.ts (I think you already did)
   • isCellFile, isPageFile, isGraphQLSchemaFile etc. in internal/files. (This functionality may have been duplicated in structure). The difference is that these functions don't just do path lookups, but often check inside the contents of the file i.e. introspection.
   • packages/internal/src/gql.ts
   • packages/internal/src/jsx.ts
   • packages/internal/src/jsxAttributeValue.ts
   • packages/internal/src/validateSchema.ts

2. What do you think of renaming this package to @redwoodjs/introspection

3. For the heavy validations (like validateSchema, or anything that imports from gql codegen) could we do "import proxying" and remove the index.ts/js - like we do here: packages/api/cache

   This is because we're trying to make sure the heavier dependencies don't get loaded in memory unless they're directly used.

4. It would be nice (if possible) to organise the validations e.g. printDiagnostics and validateSchema away from the "skeleton" stuff and away into a separate folder(s). We have to balance grouping i.e. putting into one folder vs the import cost.

   So for example, if I am calling printDiagnostics, none of the graphql-codegen related dependencies should be in memory.

Hopefully this all makes sense, otherwise we can link up soon and get this in!