-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Make Wasp work with tsconfig path aliases #2457
base: main
Are you sure you want to change the base?
Conversation
@@ -45,7 +45,7 @@ | |||
{=! expose it here (which leaks it to our users). We could avoid this by =} | |||
{=! using relative imports inside SDK code (instead of library imports), =} | |||
{=! but I didn't have time to implement it. =} | |||
"./ext-src/*": "./dist/ext-src/*.js", | |||
"./src/*": "./dist/src/*.js", |
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.
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.
What do you mean by "renaming aliases", does that mean you'd search & replace src
with ext-src
in the SDK version of the files?
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.
Yes, both there and in tsconfig paths
.
esbuild({ | ||
target: 'esnext', | ||
}), | ||
], | ||
// We don't want to bundle any of the node_module deps | ||
// as we want to keep them as external dependencies | ||
external: (id) => !/^[./]/.test(id), | ||
external: /node_modules/, |
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.
Todo: leave a note about resolution, wasp, and bundling dependencies.
Todo: leave a note about why we're using alias
and not some of the other plugins (check notes).
@infomiho I plan to explain this in detail. It's all in my notes.
{=# paths =} | ||
"{= path =}": ["{= lookupLocation =}"], | ||
{=/ paths =} | ||
} |
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.
When the user doesn't define any aliases, this file still has an empty object.
I thought that was cleaner than doing the conditional check, but feel free to disagree :)
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.
Looking at the compiler source code it doesn't seem like it will cause any issues 👍
@@ -12,6 +12,7 @@ import { join as joinPaths } from 'path' | |||
* .wasp/out/web-app directory. This function resolves a project root dir path | |||
* to be relative to the `web-app` directory i.e. `../../../projectDirPath`. | |||
*/ | |||
// TODO: The relative path should come from Haskell |
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.
I'll either solve this for the next RC or create an issue.
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.
Since it's old code, I think it's okay if we just create an issue for this.
entries: [ | ||
{=# aliases =} | ||
{ find: '{= find =}', replacement: '{= replacement =}' }, | ||
{=/ aliases =} | ||
] |
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.
NOTE: Just like paths
in sdk/tsconfig
, this field's value is an empty array when the user doesn't define any aliases.
"@util": ["./src/util.js"], | ||
"@components/*": ["./src/components/*"], |
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.
I wanted to test aliases with and without wildcards.
Todo: update headless.
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.
Todo: what about more wildcards on the left side?
@@ -69,6 +70,7 @@ data AppSpec = AppSpec | |||
prismaSchema :: Psl.Schema.Schema, | |||
-- | The contents of the package.json file found in the root directory of the wasp project. | |||
packageJson :: PackageJson, | |||
tsConfig :: TsConfig, |
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.
I need to carry the path information all the way to the generator, but I'm not using anything else from the object.
Should I have only sent that?
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.
I had the same dilemma with prismaSchema
since I needed only entities and not the config bits. I ended up needing the whole thing after all and when I rewrote it to include the full prismaSchema
- the code felt easier to understand after I did that. I like the mental model of "we have all the project info in AppSpec that we might".
When somebody looks at AppSpec, I think it's easier to understand "Oh yes, the project tsconfig
" is available vs. there is this specific paths object that we pass around. That prompts me to maybe rename it to srcTsConfig
or smth like that since it might be named tsconfig.src.json
as well.
data ImportPathMapping | ||
= ImportPathMapping (Map String String) | ||
deriving (Show, Generic, ToJSON) |
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.
TS config path aliases point to arrays:
"paths" : {
"@util": ["./src/util.js"],
"@components/*": ["./src/components/*"],
}
Rollup aliases point to single strings:
alias({
entries: [
{ find: '@components', replacement: '.././src/components' },
{ find: '@util', replacement: '.././src/util.js' },
]
}),
TS supports arrays because it can look for paths sequentially until it finds the correct one. Rollup can't do that.
Rollup therefore limits our entire system to one-on-one path mappings. So, I've decided to catch that as early as possible (i.e., while parsing TS config).
I should have arguably done this while validating the TS config. But then I'd have to introduce a new object (something like ValidTsConfig
), and change the validateSrcTsConfig
function to return it (it would then technically stop being a validator and become a constructor/parser).
I didn't do this because I wasn't sure it was the right choice and it would probably take longer.
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.
I should have arguably done this while validating the TS config.
Yep, this feels like a better place to perform this logic. This doesn't feel like JSON parsing logic to me.
We have this case already in the code base, the case of AppSpec
being validated in Valid.hs
and then we continue to use the same AppSpec
object knowing it's validated. I'm not saying that's the best, but we survived :)
I'm okay with keeping this here as well, it's preventing us from constructing a faulty TsConfig - which is nice.
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.
The only behavioral change here is validating the "baseUrl"
.
The rest of the changes are refactoring:
- Reorganized some of the functions and arguments to make them more composable (e.g., for validating stuff that's not in
compileOptions
. - Changed some names.
- Made small changes to the error messages to make them sound more human.
@infomiho Hope you'll like it :)
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.
I like the changes :)
fieldMustBeUnsetErrorMessage = | ||
unwords | ||
[ "The", | ||
"\"" ++ show fullyQualifiedFieldName ++ "\"", | ||
"field in tsconfig.json must be unset." | ||
] |
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.
Todo: message is weird, change it.
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.
Left some comments. I think improving the error messages are what I'd like to see the most. Next are some unit tests for the alias transformations.
{=# paths =} | ||
"{= path =}": ["{= lookupLocation =}"], | ||
{=/ paths =} | ||
} |
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.
Looking at the compiler source code it doesn't seem like it will cause any issues 👍
@@ -1,3 +1,6 @@ | |||
export function sayHi() { | |||
console.log("This is coming from shared function.") | |||
} | |||
|
|||
|
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.
Redundant new lines 🤷
@@ -69,6 +70,7 @@ data AppSpec = AppSpec | |||
prismaSchema :: Psl.Schema.Schema, | |||
-- | The contents of the package.json file found in the root directory of the wasp project. | |||
packageJson :: PackageJson, | |||
tsConfig :: TsConfig, |
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.
I had the same dilemma with prismaSchema
since I needed only entities and not the config bits. I ended up needing the whole thing after all and when I rewrote it to include the full prismaSchema
- the code felt easier to understand after I did that. I like the mental model of "we have all the project info in AppSpec that we might".
When somebody looks at AppSpec, I think it's easier to understand "Oh yes, the project tsconfig
" is available vs. there is this specific paths object that we pass around. That prompts me to maybe rename it to srcTsConfig
or smth like that since it might be named tsconfig.src.json
as well.
|
||
makeMultipleLocationsErrorMsg :: [(String, NonEmpty String)] -> String | ||
makeMultipleLocationsErrorMsg locations = | ||
"One or more paths point to multiple lookup locations: " |
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.
I'd maybe rewrite this to be a bit more clear:
Current:
One or more paths point to multiple lookup locations: [("@/*",["./src/*","./.wasp/generated/*"])]. Wasp only supports one-on-one path mappings.
Proposed:
One or more paths point to multiple lookup locations:
"@/*": ["./src/*","./.wasp/generated/*"]
Wasp only supports one-on-one path mappings.
This matches what they see in their tsconfig better.
data ImportPathMapping | ||
= ImportPathMapping (Map String String) | ||
deriving (Show, Generic, ToJSON) |
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.
I should have arguably done this while validating the TS config.
Yep, this feels like a better place to perform this logic. This doesn't feel like JSON parsing logic to me.
We have this case already in the code base, the case of AppSpec
being validated in Valid.hs
and then we continue to use the same AppSpec
object knowing it's validated. I'm not saying that's the best, but we survived :)
I'm okay with keeping this here as well, it's preventing us from constructing a faulty TsConfig - which is nice.
unwords | ||
[ "Invalid value for the", | ||
"\"" ++ show fullyQualifiedFieldName ++ "\"", | ||
"field in tsconfig.json, you must set it to:", |
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.
Sometimes it's tsconfig.json
, and other times tsconfig.src.json
if I'm not mistaken?
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.
I like the changes :)
makeAliasTemplateData (path, lookupLocation) = | ||
Aeson.object | ||
[ "find" .= stripWildcardPath path, | ||
"replacement" .= (SP.fromRelDir waspProjectDirFromSrcDir </> stripWildcardPath lookupLocation) |
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.
I'd like to see some examples in comments above or even better unit tests for this transformation :)
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.
Will definitely add those.
Also, I have to figure out whether prefixing with ../
works for aliases in nested files.
@@ -37,4 +37,4 @@ readTsConfigFile :: Path' Abs (File f) -> IO (Either String T.TsConfig) | |||
readTsConfigFile tsConfigFile = do | |||
tsConfigContent <- IOUtil.readFileBytes tsConfigFile | |||
parseResult <- parseJsonWithComments . BS.toString $ tsConfigContent | |||
return $ left ("Failed to parse tsconfig file" ++) parseResult | |||
return $ left ("Failed to parse tsconfig file: " ++) parseResult |
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.
I feel like saying "tsconfig.json"
or "tsconfig.src.json"
would help users understand where to look.
"outDir": ".wasp/out/user", | ||
"paths" : { | ||
"@util": ["./src/util.js"], | ||
"@components/*": ["./src/components/*"], |
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.
Did you use these aliases anywhere in the headless tests?
Also, maybe we could also use alises in the e2e tests (this is probably a "Create an issue for later" candidate)
No description provided.