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

chore: resolve minor feedback from #4685 #4700

Merged
merged 6 commits into from
Oct 25, 2024
Merged

Conversation

divmain
Copy link
Contributor

@divmain divmain commented Oct 25, 2024

Details

#4685 got merged automatically with a few comments still pending. However, only very minor changes were remaining. Those comments are addressed here.

Does this pull request introduce a breaking change?

  • 😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • 🤞 No, it does not introduce an observable change.

GUS work item

W-17029474

import { transmogrify } from '../transmogrify';
import type { Program as EsProgram } from 'estree';

const COMPILED_CMP = `
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like it's gonna get outdated real fast. 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, could we just have placeholder/stubs here instead? Literally just an async function* tmpl() {} should be enough to test the basics of transmogrification. Alternatively, call the compiler itself to generate this string.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Although now that I look at it... this is pretty hello-world-ish. So maybe it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I took the most basic fixture we had, modified the customer code to add things like customer-written async generator functions, compiled, and stripped out the parts of the compiled output that I didn't really care about. What is left is not actually runnable code, but retains the pieces that I wanted to make assertions about.

Copy link
Contributor Author

@divmain divmain Oct 25, 2024

Choose a reason for hiding this comment

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

In the future, it might be worth adding some of these customer-written async generator things (or whatever) into a fixture somewhere and actually consuming that stuff programmatically in a way that would effect rendering. Those might be useful additional guardrails that would be a bit more "evergreen" compared to this snippet of code.

Copy link
Collaborator

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

I'd still like to see ssrMode folded into the main set of options, but it's not a big deal since this mode may be temporary anyway.

@divmain
Copy link
Contributor Author

divmain commented Oct 25, 2024

I'd still like to see ssrMode folded into the main set of options, but it's not a big deal since this mode may be temporary anyway.

I'll take a look at this in another follow-up PR. It'll touch packages other than the SSR-related ones, and will probably be easier to review atomically.

@divmain divmain merged commit 75ff766 into master Oct 25, 2024
11 checks passed
@divmain divmain deleted the divmain/followup-4685 branch October 25, 2024 20:37
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.

3 participants