Bug Fix: store typescript_declarations config and rebuild if changed #4523
Conversation
daniellionel01
changed the title
| @@ -370,6 +370,9 @@ | |||
| segment would generate invalid code on the Erlang target. | |||
| ([Surya Rose](https://github.com/GearsDatapacks)) | |||
|
|
|||
| - Fixed a bug where enabling `typescript_declarations = true` wouldn't generate TypeScript declarations unless the build directory was manually deleted. The compiler now automatically rebuilds the project when this configuration changes. | |||
| ([daniellionel01](https://github.com/daniellionel01)) | |||
There was a problem hiding this comment.
Wrap long lines to match the existing entries please
| @@ -0,0 +1,14 @@ | |||
| name = "javascript_typescript_declarations" | |||
There was a problem hiding this comment.
This test project doesn't seem to be used anywhere? What is it for?
Could be a good foundation for an integration test in a CI workflow.
There was a problem hiding this comment.
I'm not very familiar with testing in rust, so I am not sure how to setup a testing workflow here. I've removed the project though
| if !self.io.is_file(&path) { | ||
| self.io.write(&path, crate::javascript::PRELUDE_TS_DEF)?; | ||
| } | ||
| self.io.write(&path, crate::javascript::PRELUDE_TS_DEF)?; |
There was a problem hiding this comment.
Has been reverted, not sure what I thought there
| @@ -261,6 +264,62 @@ where | |||
| }) | |||
| } | |||
|
|
|||
| /// Checks if the TypeScript declarations setting has changed since the last build. | |||
| /// If it has changed, we clear the build directory to make sure it generates TypeScript definitions. | |||
| pub fn check_javascript_config(&self) -> Result<(), Error> { | |||
There was a problem hiding this comment.
Let's give this a more descriptive name that says what it does.
| pub fn check_javascript_config(&self) -> Result<(), Error> { | ||
| if !self.target().is_javascript() { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
We also only care about this if configured to perform code generation, otherwise it does nothing.
| .build_directory_for_target(self.mode(), self.target()); | ||
| let config_path = build_path.join("gleam-toml.lock"); | ||
|
|
||
| let current_setting = format!( |
There was a problem hiding this comment.
Always favour the declarative serde derive API over ad-hoc serde usage.
That said, we could just write an empty file and its existence can indicate that TS was asked for. Then we don't need to do any reading or parsing.
There was a problem hiding this comment.
done 👍 the file is now called typescript-declarations.lock
| kind: FileKind::File, | ||
| path: config_path, | ||
| err: Some(e.to_string()), | ||
| }) |
There was a problem hiding this comment.
There's a bug here! The build directory doesn't have all the contents it needs to have after being reset.
We can fix it by not deleting the entire directory. It would be more proper for this compiler instance to only delete the sub-directory it owns.
There was a problem hiding this comment.
I'm not 100% sure if I understand this correctly. So right now it deletes the build/dev/javascript directory, but that shouldn't happen? By "sub-directory it owns" are you referring to build/dev/javascript/[project name]? Because from what I am able to tell we also need to generate the .d.mts files inside of the adjacent directories f.e. gleam_stdlib.
We could also delete all of the *.mjs files inside of the build/dev/javascript directory, would that be better?
| @@ -188,6 +188,9 @@ where | |||
| // verify that this version is appropriate. | |||
| self.check_gleam_version()?; | |||
|
|
|||
| // Check if JavaScript configuration has changed | |||
There was a problem hiding this comment.
More detail in the documentation please 🙏
@lpil addressed all comments, thank you! Will be awaiting your feedback 🤘 I couldn't get the test for CI working yet, but have tested it locally quite a bit |
|
Could you rebase to remove the merge commit please 🙏 |
daniellionel01
force-pushed
the
bugfix/typescript-declarations
branch
from
3f0c1c7 to
b5af4fd
Compare
@lpil I'll be honest, I've never rebased anything and am failing hard to do so 🥲 I've tried for half an hour with GPT and messed up the branch like 3 times already. Any help is appreciated. It might be faster for me to just copy paste my changes (since it's just the one function) to a fresh branch, close this PR and open a new one |
Code to fix #4520
It currently creates a file
gleam-toml.lockwhere it stores the configuration fortypescript_declarations. If that has changed since the last build, itrm -rfs the build directory to make sure that it emits typescript declaration files this time. Might be a bit overkill and only necessary to for examplerm -rfthe dev/javascript directory, I am not sure.Has been tested locally in a dedicated gleam project. But I can add something to the test suite to make it part of CI/CD if that is a requirement.
So in summary, some aspects that should be discussed in more depth:
rm -rfthe whole build directory or only parts of it? for examplebuild/dev/javascript?gleam-toml.lockfilegleam-config.lock,gleam.toml,config.lock, ...)Cheers!