-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix: removes warning about --experimental flags #1
base: main
Are you sure you want to change the base?
Conversation
…t#531) Adds the templatesJson to the header so that we can access it in the Visual Editor. Tested locally and verified that the templatesJson was present in a script tag in the header. Also verified that `npm run prod` still works as expected and the templatesJson is not present.
This provides a post request option for `serverRenderRoute` which overrides the c_visualLayouts field of the pulled document with the overrides that are passed in from the request. --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Matt Kilpatrick <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR introduces significant changes to the @yext/pages package, focusing on template generation, hydration process improvements, and server-side rendering enhancements. Here are the key points:
- Removed custom loader and experimental flags from Node.js process spawn, simplifying execution and eliminating warnings.
- Added new template generation functionality, including support for Visual Editor, dynamic, and static templates.
- Enhanced hydration process to include template configurations in the client-side code.
- Modified server-side rendering to allow document overrides via POST requests.
- Introduced new parsers for Puck configuration files and improved source file parsing capabilities.
- Updated the scaffold command to include a new 'template' subcommand for easier template creation.
22 file(s) reviewed, 16 comment(s)
Edit PR Review Bot Settings
* [email protected] (2953ca7a) | ||
* [email protected] (dfb04527) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Duplicate version entry. Consider removing one
if (fs.existsSync("test.tsx")) { | ||
fs.unlinkSync("test.tsx"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Use fs.promises.unlink for consistent async operations
if (!fs.existsSync(filepath)) { | ||
throw new Error(`Filepath "${filepath}" is invalid.`); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider using a more specific error message that includes the expected file path format.
const formattedTemplateName = | ||
fileName.charAt(0).toUpperCase() + fileName.slice(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider using fileName.charAt(0).toUpperCase() + fileName.slice(1).toLowerCase()
to ensure consistent formatting.
getVariableStatement(variableName: string) { | ||
return this.sourceFile.getVariableStatementOrThrow(variableName); | ||
} | ||
|
||
getVariableDeclaration(variableName: string) { | ||
return this.sourceFile.getVariableDeclarationOrThrow(variableName); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: These methods throw errors if the variable is not found. Consider adding error handling or returning undefined instead
import fs from "node:fs"; | ||
import { Project } from "ts-morph"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider using fs/promises for asynchronous file operations
if (fs.existsSync("test.tsx")) { | ||
fs.unlinkSync("test.tsx"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Move file cleanup to a separate function for reusability
); | ||
const filePath = "test.tsx"; | ||
|
||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Duplicate code structure across tests. Consider extracting to a helper function
} catch (error) { | ||
logErrorAndExit(error); | ||
} | ||
process.exit(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider removing explicit process.exit(0). Node.js will exit automatically when the event loop is empty.
{ | ||
"$id": "closed-location-redirects", | ||
"fields": [ | ||
"slug" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider including more fields for closed locations if needed for redirect logic
Tested locally with node 18 and 20
Tested in platform with a starter site as well.
PR that added this code initially