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

Add support for gjs/gts files #648

Merged
merged 3 commits into from
Jan 30, 2024
Merged

Conversation

vstefanovic97
Copy link
Contributor

How this works:

@drbzyx
Copy link

drbzyx commented Dec 19, 2023

@Mikek2252 Hey, would you be able to review this PR? It would be great to have support for the new Template Tag components.

@Mikek2252
Copy link
Collaborator

@vstefanovic97 Looks mostly fine, but i believe globby returns a promise so the functions that return globby values should be awaited.

@vstefanovic97
Copy link
Contributor Author

@Mikek2252 they are all awaited, only thing I did was remove extraneous async on function that just return a promise but the don't have any await's inside them.
The places where I removed await from function invocations are where functions don't return a promise at all

@Mikek2252
Copy link
Collaborator

@vstefanovic97 My mistake, this looks fine to me, i'll merge this now and get a release out in the next few days

@vstefanovic97
Copy link
Contributor Author

Thank you @Mikek2252!

@Turbo87
Copy link
Collaborator

Turbo87 commented Dec 19, 2023

only thing I did was remove extraneous async on function that just return a promise but the don't have any await's inside them.

this leads to errors thrown from these functions to not be converted to promise rejections though, so this needs to be done very carefully. it usually better to keep them as async functions and use return await ... inside.

@vstefanovic97
Copy link
Contributor Author

vstefanovic97 commented Dec 19, 2023

@Turbo87 I think that is probably only a problem if you are using then/catch syntax and the function ends up throwing an error and it doesn't get handled by catch.

But if you are using try/catch with async/await to handle error/promise rejection then it should be fine since still we will catch the error.

Either way I don't see a difference where this can affect the current logic, but if you prefer I can add back async keyword to the function to make sure they always return a promise?

@Turbo87
Copy link
Collaborator

Turbo87 commented Dec 19, 2023

@vstefanovic97 it is a problem because it breaks the assumption about Promise-returning functions. aside from that, the async keyword is also a way to communicate to other developers that this function returns a Promise. without the keywords you have to dive into the internals of the function to figure out the return types.

tl;dr removing the async keyword can cause many subtle and often invisible issues

@Turbo87
Copy link
Collaborator

Turbo87 commented Dec 19, 2023

and if you don't believe me, take it from the W3C: https://www.w3.org/2001/tag/doc/promises-guide#always-return-promises 😉

@vstefanovic97
Copy link
Contributor Author

@Turbo87 I see your point, I added back the async keyword, do you mind running the workflow again?

Comment on lines -180 to +191
async function analyzeFiles(cwd, files, options) {
function analyzeFiles(cwd, files, options) {
let allTranslationKeys = new Map();

for (let file of files) {
let translationKeys = await analyzeFile(cwd, file, options);
let translationKeys = analyzeFile(cwd, file, options);
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like there are still plenty of places where async/await was removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Turbo87 those don't actually return promises, they are sync functions.
So basically for some reason sync functions had async keyword which I would say is also confusing

@drbzyx
Copy link

drbzyx commented Jan 18, 2024

Hey, sorry to be a pain but are there any updates on this PR?

@Mikek2252
Copy link
Collaborator

Apologies, @Turbo87 are you happy with the updates? if so i'll get this merged and create a release

@vstefanovic97
Copy link
Contributor Author

vstefanovic97 commented Jan 24, 2024

Pinging @Turbo87 again, are we ok with the changes now? cc: @Mikek2252

@amk221
Copy link

amk221 commented Jan 29, 2024

Would love to get a release with this 💛

@Mikek2252
Copy link
Collaborator

To avoid anymore delays, im going to merge and create a release for this feel free to reach out @Turbo87 if you need me to update anything

@Mikek2252 Mikek2252 merged commit 6783922 into mainmatter:master Jan 30, 2024
5 checks passed
@Mikek2252 Mikek2252 added the enhancement New feature or request label Jan 30, 2024
@Mikek2252
Copy link
Collaborator

@vstefanovic97 Thank you for the contribution, version 4.5.0 has been released now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants