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

feat: add check on zk parser #28

Conversation

itsacoyote
Copy link
Contributor

@itsacoyote itsacoyote commented Apr 23, 2024

What πŸ’»

  • Add a console error if a zk tag is unparsed because of bad formatting

Why βœ‹

  • Avoid displaying any unformatted/broken tags in production

Evidence πŸ“·

Screenshot 2024-04-23 at 3 46 14β€―PM

NOTE

This does not stop a build process from happening. The parser runs on dev and if an error is Thrown it causes all sorts of hiccups on the local server to restart smoothly. For now this is just a console log for the developer to notice. May sit on this and think of a way to throw an error only on build later on.

@itsacoyote itsacoyote marked this pull request as ready for review April 23, 2024 19:49
@itsacoyote itsacoyote requested a review from a team as a code owner April 23, 2024 19:49
Copy link

github-actions bot commented Apr 23, 2024

Visit the preview URL for this PR (updated for commit a76a332):

https://zksync-docs-staging-5eb09--pr28-itsacoyote-devrl-487-wuthvex2.web.app

(expires Tue, 07 May 2024 14:23:22 GMT)

πŸ”₯ via Firebase Hosting GitHub Action 🌎

Sign: bfaafba5fa82d4f63473aaa76a21fabf1fbb3a11

@itsacoyote
Copy link
Contributor Author

Orrrr do we just want to throw a straight up Error and break the server if an unparsed tag is found?

@dutterbutter
Copy link
Contributor

@itsacoyote I think this fine, it will be identified in the CI anyway right? So we will still be aware of the need to fix.

@dutterbutter
Copy link
Contributor

Actually I don't think it would be identified by the CI?

@itsacoyote
Copy link
Contributor Author

Actually I don't think it would be identified by the CI?

It will not, this is part of the dev process which is why I was worried about throwing an Error in it. While you're developing locally and it throws, it throws a bit of a wrench in the local server and makes it a pain to start back up.

I'll have to look a bit further into the lifecycle of the build process and see if I can find a way to throw only during a build command.

@itsacoyote itsacoyote force-pushed the itsacoyote-devrl-487-add-after-parse-check-on-tag-replacement branch from 7793901 to 3c7e236 Compare April 24, 2024 15:51
@itsacoyote
Copy link
Contributor Author

itsacoyote commented Apr 24, 2024

@dutterbutter I've got an error that I can throw and make it fatal but it still doesn't cleanly exit the process with a proper error code. The failure is because the rest of the build process gets borked which isn't really the best way. I'd have to dig into this further but don't want to waste more time on it right now, what are your thoughts on how we should handle this for now?

An error does occur in the Deploy target: staging step but you can see in the action workflow that it looks "successful" and what errors is the link checker. This is because the build gets killed but the error got swallowed up somewhere.

@dutterbutter
Copy link
Contributor

Agreed, let's not spend too much time on this right now. Let's sit on this and think through later, perhaps during offsite. Feel free to keep as is or move to draft state.

@itsacoyote itsacoyote closed this Apr 25, 2024
@itsacoyote itsacoyote reopened this Apr 25, 2024
@itsacoyote itsacoyote marked this pull request as draft April 25, 2024 16:12
@itsacoyote itsacoyote force-pushed the itsacoyote-devrl-487-add-after-parse-check-on-tag-replacement branch 2 times, most recently from 5dcc71b to a76a332 Compare April 30, 2024 14:19
@itsacoyote itsacoyote marked this pull request as ready for review May 1, 2024 09:09
@itsacoyote itsacoyote merged commit f472a65 into staging May 6, 2024
7 checks passed
@itsacoyote itsacoyote deleted the itsacoyote-devrl-487-add-after-parse-check-on-tag-replacement branch May 6, 2024 13:56
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