-
Notifications
You must be signed in to change notification settings - Fork 49.3k
[compiler][playground] (1/N) Config override panel #34303
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
[compiler][playground] (1/N) Config override panel #34303
Conversation
2351552
to
2838933
Compare
@@ -328,6 +329,7 @@ export default function Editor(): JSX.Element { | |||
return ( | |||
<> | |||
<div className="relative flex basis top-14"> | |||
<ConfigEditor /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about including this conditionally for now? Something like a URL param could enable it. That way we can land this in its current state. Since its still read-only it may be confusing to see it without any config pragmas applied in the main editor.
if (!value) return; | ||
|
||
// TODO: Implement sync logic to update pragma comments in the source | ||
console.log('Config changed:', value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's disable the editor while we don't have this hooked up. Output tab does this for example. In a future PR when the handler is added it can be re-enabled.
/** | ||
* Format the config object as readable JavaScript, assigned to a const | ||
*/ | ||
function formatConfigAsJavaScript(config: any): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Output.tsx uses prettier.format
. Can that be used here instead of the custom formatting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually discovered I may have to redesign the current implementation, starting by overwriting how pragmas are written. Right now we have to convert from a JS Record type to a source code string, which requires manual formatting for complicated types. I will have to modify the pragma structure to look basically like JS code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right I was misunderstanding the conversion here. The input is the pragma which of course needs custom formatting in both directions. Seems fine to start with this.
This is exciting! Is the idea to keep this as a JSON format, or eventually switch to a graphical format? My initial reaction is that you rarely need to edit config, so it should not take up screen space by default. Can we hide this behind a drop down menu? |
Right now we wish to keep it in a JS format actually (to account for Function & Map types). Ideally if there are any config overrides, you would be able to easily copy-paste into the editor and see the changes. Do you think there would be a big benefit in a graphical format as well?
Yes, planning to do this |
No strong opinion, there are pros/cons either way |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work getting this sketched out! Looking forward to seeing the next iterations.
There is some failing test signal to clean up.
/** | ||
* Format the config object as readable JavaScript, assigned to a const | ||
*/ | ||
function formatConfigAsJavaScript(config: any): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right I was misunderstanding the conversion here. The input is the pragma which of course needs custom formatting in both directions. Seems fine to start with this.
if (typeof window !== 'undefined') { | ||
const urlParams = new URLSearchParams(window.location.search); | ||
setShouldShowConfig(urlParams.get('showConfig') === 'true'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be handled by useSearchParams()
https://nextjs.org/docs/app/api-reference/functions/use-search-params
If this is currently a server component, you could just use the hook in ConfigEditor
and make that the client boundary. Returning null if there's no param.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's double check on the package export but otherwise this looks good to land and iterate on.
The override pragma looks ok to move forward with. Its additive so it won't get in anyones way and we can always adjust it.
const [configJavaScript, setConfigJavaScript] = useState(''); | ||
|
||
useEffect(() => { | ||
const pragma = store.source.substring(0, store.source.indexOf('\n')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could compare this against the previous value to bail out of the effect early if you don't need to sync again.
const previousPragma = useRef()
useEffect(()=> {
const pragma = store.source.substring(0, store.source.indexOf('\n'));
if (pragma === previousPragma.current) {
return;
}
previousPragma.current = pragma;
// ...
}, [])
An alternative approach (can be a followup PR) is to avoid this effect altogether by moving this into the event. Currently the flow is
- User updates editor content
- Content onChange fires
- editor state update dispatch to context
- useStore() is subscribed to changed context
- ConfigEditor rerenders
- effect fires
- prettier runs, sets state
- ConfigEditor rerenders
If we refactor the store to have a config property, it could be
- User updates editor content
- Content onChange fires
- pragma is parsed, if it has changed format and dispatch the updated config state in a callback
- ConfigEditor renders latest state from context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestions! Yea definitely planning to fix the re-renders in a later commit, also going to try to fix the rerenders of the other components that use useEffect and useMemo incorrectly.
export {parseConfigPragmaForTests} from './Utils/TestUtils'; | ||
export { | ||
parseConfigPragmaForTests, | ||
parseConfigPragmaAsString, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we want this to be a top-level export of the compiler package. Especially if this logic is specific to the playground app. Though I see parseConfigPragmaForTests
so @mofeiZ might have a better opinion.
Seems like how you had it before in the ConfigEditor component module would be ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compiler package exposes a lot of implementation details already in its exports so either is fine 😅
It isn't necessary to expose this as a top level export though, you can import directly from a compiler source file. This is because next-js (e.g. compiler playground) does its own bundling which means it currently recursively traverses babel-plugin-react-compiler
source files instead of using the pre-bundled version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
For the test complex config defaults, feel free to remove that logic entirely from parseConfigString
. We can replace this with referencing a (magically in-scope) global config as needed
// @OVERRIDE: { environment: { inlineJsxTransform: TEST_CONFIG.inlineJsxTransform } }
Summary
Part 1 of adding a "Config Override" panel to the React compiler playground. The panel is placed to the left of the current input section, and supports converting the comment pragmas in the input section to a JavaScript-based config. Backwards sync has not been implemented yet.
NOTE: I have added support for a new
OVERRIDE
type pragma to add support for Map and Function types. (For now, the old pragma format is still intact)Testing
Example of the config overrides synced to the source code:
