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

Fix generate-chains script to work with Prettier v3 #67

Merged
merged 5 commits into from
Aug 23, 2023

Conversation

andreogle
Copy link
Member

Apologies I missed that upgrading to Prettier v3 broke the generate-chains script. I've adjusted the script to be async now that the exposed Prettier functions return promises.

@andreogle andreogle added the bug Something isn't working label Aug 23, 2023
@andreogle andreogle self-assigned this Aug 23, 2023
Copy link
Contributor

@dcroote dcroote left a comment

Choose a reason for hiding this comment

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

Does watchJsonFiles also need updating given the calls to the now async mergeJsonFiles?

@andreogle
Copy link
Member Author

Does watchJsonFiles also need updating given the calls to the now async mergeJsonFiles?

I thought it might, but it appears to work just fine without any changes. i.e. I can run both yarn generate:chains and yarn generate:chains --watch with both working as expected.

@andreogle
Copy link
Member Author

I'm merging this so I can release v3.4.0 with a working generate:chains script. Will make any further adjustments in follow up PRs

@andreogle andreogle merged commit cf3d3ee into main Aug 23, 2023
3 checks passed
@andreogle andreogle deleted the fix/async-prettier branch August 23, 2023 15:15
@dcroote
Copy link
Contributor

dcroote commented Aug 23, 2023

Sounds good to merging. My last thought on the discussion:

with both working as expected.

When watching, the files are actually merged (not just there is a console message)? I always find it non intuitive as to when functions return vs actually execute

@andreogle
Copy link
Member Author

When watching, the files are actually merged (not just there is a console message)?

I'm not sure I understand what you mean here, sorry. Would you mind elaborating a bit? Are you suggesting another change to how the watching is currently setup?

For what it's worth, I'm not actually even sure that it's worth having the --watch option 🤔 Someone working on one or more of the actual chain JSON files doesn't need the exported CHAINS object to be constantly updated. It's easy enough to just run yarn generate:chains once done (and there are several things in place that will let you know if you haven't run it).

@dcroote
Copy link
Contributor

dcroote commented Aug 24, 2023

Would you mind elaborating a bit?

Haha yes, that was vague. I meant that it could be the script looks like it's working because async calls return immediately without await and therefore the files may not be merged, which is akin to what was happening in the comment I linked to. But it could also be working fine 🤷‍♂️

Sounds like it would also be fine without watch at all though, and I always love less code haha

@andreogle
Copy link
Member Author

andreogle commented Aug 25, 2023

I always love less code haha

Agreed 🤝 I'm going to remove the watch option

I don't think the async call functions are an issue, since this script runs in < 1 second (although I get what you mean now, thanks)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants