Address final review of #2638 #2788

cprecioso · 2025-05-22T08:26:35Z

Review changes for #2638

…yxotlm

Martinsos

Left some comments @cprecioso , I like where we are going, I think we are quite closer to a finished thing!

Except for what I commented on, one main thing that I am suspicious a bit of is the current LanguageInfo situation. That whole part is certainly better now, as you said it is more explicit, there are less hidden assumptions. But on the other hand, this centralization of LanguageInfo seems a bit weird to me, a bit artifical and too emphasized. I tryed playing a bit with replacing that structure with functions like function tsToJsExt (tsExt: "ts" | "tsx"): "js" | "jsx" and function tsExtIsJsx (tsExt: "ts" | "tsx"): boolean but I am not sure that is the right direction either.
Maybe that is ok all together -> I would say, let's take care of everyting that is outstanding, then at the very end we can take one last look at the LanguageInfo situation and see if we want to do something there or not, it will be clearer situation then.

web/README.md

web/docs/tutorial/07-auth.md

web/src/remark/auto-js-code.ts

web/src/remark/code-with-hole.ts

web/src/remark/util/code-blocks.ts

web/src/remark/util/prettier.ts

…yxotlm

cloudflare-workers-and-pages · 2025-05-27T10:23:44Z

Deploying wasp-docs-on-main with Cloudflare Pages

Latest commit:	`b8d75e7`
Status:	✅ Deploy successful!
Preview URL:	https://5a5f0985.wasp-docs-on-main.pages.dev
Branch Preview URL:	https://cprecioso-push-lxzuqtyxotlm.wasp-docs-on-main.pages.dev

View logs

cprecioso · 2025-05-27T12:06:03Z

@Martinsos addressed the comments, please review

I tryed playing a bit with replacing that structure with functions like function tsToJsExt (tsExt: "ts" | "tsx"): "js" | "jsx" and function tsExtIsJsx (tsExt: "ts" | "tsx"): boolean but I am not sure that is the right direction either.

I think Haskell-isms are leading you on here, I wouldn't say this kind of solution is idiomatic JS/TS at all.

seems a bit weird to me, a bit artifical and too emphasized

Well, yes 😅 If we want to operate at this level of abstraction, we need a more-or-less central way of defining the transformation. And right now, it does look like overkill for just two possible alternatives. If we want to make it simpler I think I'd end up slipping back to something very similar to what we had.

Martinsos · 2025-05-29T13:47:57Z

@Martinsos addressed the comments, please review

I tryed playing a bit with replacing that structure with functions like function tsToJsExt (tsExt: "ts" | "tsx"): "js" | "jsx" and function tsExtIsJsx (tsExt: "ts" | "tsx"): boolean but I am not sure that is the right direction either.

I think Haskell-isms are leading you on here, I wouldn't say this kind of solution is idiomatic JS/TS at all.

seems a bit weird to me, a bit artifical and too emphasized

Well, yes 😅 If we want to operate at this level of abstraction, we need a more-or-less central way of defining the transformation. And right now, it does look like overkill for just two possible alternatives. If we want to make it simpler I think I'd end up slipping back to something very similar to what we had.

Yes it is probably a bit of Haskellisms as you said :D. I do find Haskell often to influence my code in other languages for better though, so I don't disregard them but usually follow them, to see where it brings me.
In this case, I agree that didn't feel like the right direction at the end, as I noted above. And I completely get what you did with the current approach and think it is a nice improvement.
Still, I have something gnawing at me that there is somewhat better solution just in reach, feels like there is too much complexity. But I do have to admit I haven't managed to see it quickly, and it might be a fake hunch, so I am happy with the current solution -> it is explicit and clear and that is most important, and since it is relatively local and scoped, a bit of potentially redundant complexity / non-cohesiveness can't do much damage. And exploring something better (if there is anything at all) would probably take time that is just not worth it.

Ok, good with me, nice stuff!

Martinsos

@cprecioso ok we are very close now! I think there is nothing major left,expect for one thing that I would really want us to fix before we close this, and that is making that function for chekcing the meta flag work correctly and not give false positives.

web/src/remark/util/code-blocks.ts

web/src/remark/auto-js-code.ts

web/src/remark/util/code-blocks.ts

cprecioso · 2025-05-30T08:06:20Z

@Martinsos ready

Martinsos

@cprecioso some last improvements for the new code and I think that will be it.

web/src/remark/util/code-blocks.ts

Martinsos · 2025-05-30T14:18:28Z

web/src/remark/util/code-blocks.ts

-import type { VFile } from "vfile";
+import { formatCode, FormatCodeOptions } from "./prettier";
+
+const CODE_BLOCK_META_REGEX = /([^\s=]+)(?:="([^"]*)")?/g;


Since this regex is standalone here, and not inside the function that uses it, I would shortly document what it intends to capture, since regexes are not easy to read not get the intention from.

The comment for the parse function can work for both

Nice comment on the CODE_BLOCK_META_REGEX! Doesn't mention anything about what matching groups capture, nor does it have an example, but that ok, reader can figure that on their own also.

I am not sure what do you mean by

The comment for the parse function can work for both

I wouldn't count on parseCodeBlockMetaString being documented by comment that is on CODE_BLOCK_META_REGEX. They are close to each other right now, in the source file, but they are both top level definitions in this file, and that makes them standalone. Meaning that their implementations and contracts/APIs are not tied together and can digress at any moment, even if they stay close to each other (which also might change).

So if we think parseCodeBlockMetaString deserves a comment explaining what it expects from the metaString, and I think it does (I probably suggested it earlier? I can't remember), then there should be an explicit comment. If nothing, let's at least in that comment tell the reader to check out the CODE_BLOCK_META_REGEX, that metaString needs to satisfy it -> that will already work well enough. Because at the moment that is only implementation detail, and is not a part of the contract/API of this function, this way you make it so though.

It would be a different story if you moved that regex inside of the parseCodeBlockMetaString function, then it would be an implementation detail and any comment would be just there to make it easier to understand it, not to define the contract/API of it.

It really feels like we'd prefer the regex to be inside of the function so I did that 😜

That also works! I don't mind any of the approaches, as long as stuff has clear APIs/contracts.

RAW

cprecioso · 2025-06-02T10:38:08Z

@Martinsos ready

Martinsos

LGTM! But there check out the last comments I left, one of them will probably need handling before merging (adding comments to parse function). Then merge.

cprecioso · 2025-06-03T14:16:09Z

@Martinsos check it out

Martinsos · 2025-06-03T19:17:06Z

@Martinsos check it out

All good, and I already approved it last time, feel free to merge!

Address review

250726f

cprecioso requested a review from Martinsos May 22, 2025 08:26

cprecioso self-assigned this May 22, 2025

cprecioso mentioned this pull request May 22, 2025

Automatically create JS versions of our TS code in the docs #2638

Merged

Merge remote-tracking branch 'origin/main' into cprecioso/push-lxzuqt…

c486fde

…yxotlm

Martinsos reviewed May 22, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into cprecioso/push-lxzuqt…

b3b8290

…yxotlm

cprecioso added 11 commits May 27, 2025 12:34

Restructure meta docs

cf400d1

Fix link

7717078

Dynamic meta flag

67bc5f0

Refactor

fe677b7

Remove old comment

4f91182

More improvements

1858936

Refactor

bc51c08

Fixes

91b04c9

More fixes

1ef6f0f

DRY

be1fffa

Cwd

7a1da45

Martinsos reviewed May 29, 2025

View reviewed changes

web/src/remark/util/code-blocks.ts Outdated Show resolved Hide resolved

web/src/remark/util/code-blocks.ts Outdated Show resolved Hide resolved

web/src/remark/auto-js-code.ts Outdated Show resolved Hide resolved

web/src/remark/util/code-blocks.ts Outdated Show resolved Hide resolved

cprecioso added 4 commits May 30, 2025 10:01

Parse code block meta

ff75597

Merge branch 'main' into cprecioso/push-lxzuqtyxotlm

9f8ddb1

Restore package-lock

8f5c087

rename

2aa7da7

Martinsos reviewed May 30, 2025

View reviewed changes

cprecioso added 2 commits June 2, 2025 12:29

Bug

14ad19c

Review

0dd8acb

cprecioso added 2 commits June 2, 2025 12:34

Comment

c18fd94

Review

1dfad79

Martinsos approved these changes Jun 2, 2025

View reviewed changes

Review

9144dfd

Merge branch 'main' into cprecioso/push-lxzuqtyxotlm

b8d75e7

cprecioso merged commit 8c49210 into main Jun 4, 2025
3 checks passed

cprecioso deleted the cprecioso/push-lxzuqtyxotlm branch June 4, 2025 08:04

Uh oh!

Address final review of #2638 #2788

Address final review of #2638 #2788

Uh oh!

Conversation

cprecioso commented May 22, 2025 • edited by sodic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Martinsos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cloudflare-workers-and-pages bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying wasp-docs-on-main with Cloudflare Pages

Uh oh!

cprecioso commented May 27, 2025

Uh oh!

Martinsos commented May 29, 2025

Uh oh!

Martinsos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cprecioso commented May 30, 2025

Uh oh!

Martinsos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Martinsos May 30, 2025

Choose a reason for hiding this comment

Uh oh!

cprecioso Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

Martinsos Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

cprecioso Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Martinsos Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

cprecioso commented Jun 2, 2025

Uh oh!

Martinsos left a comment

Choose a reason for hiding this comment

Uh oh!

cprecioso commented Jun 3, 2025

Uh oh!

Martinsos commented Jun 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cprecioso commented May 22, 2025 •

edited by sodic

Loading

cloudflare-workers-and-pages bot commented May 27, 2025 •

edited

Loading