Skip to content
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

Post 0.16.0 cleanup #2490

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

Post 0.16.0 cleanup #2490

wants to merge 17 commits into from

Conversation

sodic
Copy link
Contributor

@sodic sodic commented Feb 6, 2025

No description provided.

Copy link
Contributor

@infomiho infomiho left a comment

Choose a reason for hiding this comment

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

Simple enough, approved

@sodic
Copy link
Contributor Author

sodic commented Feb 7, 2025

@infomiho Sorry, should have marked this one as Draft as it's still WIP. I'll ping you for another review :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This got moved from SdkGenerator.Server.Operations, nothing new here.

Comment on lines +20 to +21
# apps for testing
examples/ignored
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I sneaked this in because I like to have experimental apps in the scope of cabal run wasp-cli but I don't like them polluting my commit history.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just keep running cabal install when testing with apps in /tmp. Hmmm, this looks useful. I'm tempted to have a note in the waspc/README.md

Comment on lines +41 to +44
return $ left ((errorMessagePrefix ++) . indent 2) parseResult
where
errorMessagePrefix = "Failed to parse '" ++ baseTsConfigFilePath ++ "':\n"
baseTsConfigFilePath = fromRelFile (basename tsConfigFile)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the indent and the new line, the message looks like this:

❌ --- [Error] Analyzing wasp project failed: -------------------------------------

1 errors found:
- Failed to parse 'tsconfig.json':
  [eval]:15
      "composite": true,
      ^^^^^^^^^^^

  SyntaxError: Unexpected string
      at makeContextifyScript (node:internal/vm:122:14)
      at node:internal/process/execution:89:22
      at [eval]-wrapper:6:24
      at runScript (node:internal/process/execution:83:62)
      at evalScript (node:internal/process/execution:114:10)
      at node:internal/main/eval_string:30:3

  Node.js v18.20.4

Without them, it looks broken:

❌ --- [Error] Analyzing wasp project failed: -------------------------------------

1 errors found:
- Failed to parse 'tsconfig.json': [eval]:15
    "composite": true,
    ^^^^^^^^^^^

SyntaxError: Unexpected string
    at makeContextifyScript (node:internal/vm:122:14)
    at node:internal/process/execution:89:22
    at [eval]-wrapper:6:24
    at runScript (node:internal/process/execution:83:62)
    at evalScript (node:internal/process/execution:114:10)
    at node:internal/main/eval_string:30:3

Node.js v18.20.4

@sodic sodic self-assigned this Feb 18, 2025
Copy link
Contributor

@infomiho infomiho left a comment

Choose a reason for hiding this comment

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

Left a few comments

Comment on lines +20 to +21
# apps for testing
examples/ignored
Copy link
Contributor

Choose a reason for hiding this comment

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

I just keep running cabal install when testing with apps in /tmp. Hmmm, this looks useful. I'm tempted to have a note in the waspc/README.md

@@ -73,21 +73,21 @@ validateFieldValue fullyQualifiedFieldName expectedValue actualValue =
unwords
[ "Invalid value for the",
"\"" ++ show fullyQualifiedFieldName ++ "\"",
"field in tsconfig.json, you must set it to:",
Copy link
Contributor

Choose a reason for hiding this comment

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

I dislike this change because it might not be clear to user which file exactly to change if there are multiple TS config files. Can we keep the file name and display the correct one for both Wasp lang and TS based config?

extOperationImportToImportJson =
GJI.jsImportToImportJson
. Just
. applyExtImportAlias
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to apply the _ext suffix here if the extImportToJsImport function adds it as well?

@infomiho infomiho self-requested a review February 28, 2025 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants