Skip to content

fix: correct checking for runtime version installed in the project and provide example API endpoint in code snippets #230

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

Merged
merged 5 commits into from
Nov 21, 2024

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Nov 21, 2024

  • update readme and fix wrong code sample - we want to compare pathname not assign it doh
  • update snippets that become part of error message when incompatible server.ts is found
  • fix check for user-installed @netlify/angular-runtime version (current one doesn't actually do anything, because it somehow is able to resolve plugin installed in <project-dir>/.netlify/plugins/node_modules despite using paths param - probably related to require.cache (?) )

Copy link

netlify bot commented Nov 21, 2024

Deploy Preview for plugin-angular-universal-demo ready!

Name Link
🔨 Latest commit 019cf7d
🔍 Latest deploy log https://app.netlify.com/sites/plugin-angular-universal-demo/deploys/673f4a0ec231ce00084b837f
😎 Deploy Preview https://deploy-preview-230--plugin-angular-universal-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the type: bug code to address defects in shipped code label Nov 21, 2024
@pieh pieh changed the title Fix/qol fixes fix: correct checking for runtime version installed in the project and provide example API endpoint in code snippets Nov 21, 2024
@pieh pieh marked this pull request as ready for review November 21, 2024 14:12
@pieh pieh requested a review from a team as a code owner November 21, 2024 14:12
Comment on lines 34 to 35
const siteRequire = createRequire(root)
packagePath = siteRequire.resolve('@netlify/angular-runtime/package.json')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In practice this was finding the @netlify/angular-runtime installed in <project-dir>/.netlify/plugins/node_modules despite paths being just ['<project-dir'>] (likely due require cache), but this should check user-installed version - this one with createRequire seems to work in practice as intended (it was now tested with monkey patching plugin instance installed in .netlify/plugins)

mrstork
mrstork previously approved these changes Nov 21, 2024
lukasholzer
lukasholzer previously approved these changes Nov 21, 2024
Copy link

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

Noice!

serhalp
serhalp previously approved these changes Nov 21, 2024
@pieh pieh dismissed stale reviews from serhalp, lukasholzer, and mrstork via 019cf7d November 21, 2024 14:56
@@ -31,7 +32,7 @@ module.exports.getAngularVersion = getAngularVersion
const getAngularRuntimeVersion = async function (root) {
let packagePath
try {
const siteRequire = createRequire(root)
const siteRequire = createRequire(join(root, ':internal:'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the previous version ended up not working when actually installed and resulted in MODULE_NOT_FOUND (that was not immediate apparent, because we swallow the error message)

@pieh pieh merged commit 3c7171f into main Nov 21, 2024
11 checks passed
@pieh pieh deleted the fix/qol-fixes branch November 21, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants