-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: server logic for create from React component #24881
feat: server logic for create from React component #24881
Conversation
Thanks for taking the time to open a PR!
|
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.
Left some comments, I will wait for the UI before I test it more thoroughly. Great to see this coming together, this is pretty much exactly what I was expecting to see when I was writing the technical brief.
packages/data-context/package.json
Outdated
@@ -75,6 +75,7 @@ | |||
"@types/sinon-chai": "3.2.8", | |||
"@types/stringify-object": "^3.0.0", | |||
"mocha": "7.0.1", | |||
"react-docgen": "5.4.3", |
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.
One thing that might be nice to do is see how much the binary increases if we add a dependency. Just something I thought of - I'm not sure what this uses under the hood, but I'm guessing it uses babel, which might bring in a bunch of extra node modules.
I think we could check it by building the binary, and just comparing the size vs the current production 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.
yeah... when I run yarn
, it adds a ton of lines to snapshot-meta.cache.json
which leads me to believe that this brings in a bunch of other modules (although I'm not sure exactly what that file does)
packages/data-context/src/codegen/templates/react-component/react-component.ejs
Outdated
Show resolved
Hide resolved
--- | ||
|
||
import React from 'react' | ||
import { <%- componentName %> } from '<%- componentPath %>' |
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.
What happens for default export components, like CounterDefault
? Would this become import { CounterDefault } from './counter-default'
(which would be undefined). Did we find a solution for this, or are we moving forward with this as a known limitation?
It should be possible to write a quick babel parser to find the default export and compare that to the components react-docgen
found. Not sure how expensive this would be, probably not that much.
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 think we'll need a separate spec template for default export components. I'm looking into how we can figure out if a component is exported by using a custom handler with React Docgen.
Otherwise yeah we can probably do it after we parse with React Docgen
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 looked at the result the componentDefinition
which is what the handler receives and it doesn't contain enough info to determine how the component was exported. For example:
const Button = (props: {text: string}) => <button>{props.text}</button>
export { Button }
export default Button
The componentDef only contains info on const Button = (props: {text: string}) => <button>{props.text}</button>
. Rather than a custom handler, I believe a custom resolver makes sense here so we can scrape all of the exported declarations and then compare them to the returned componentDefinitions. We don't need to write our own parser since a resolver directly receives the parsed ast.
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.
Going to handle this as a separate task (to be completed before releasing this feature) #24929
This reverts commit d763e1f.
…ate-from-react-server
…om/cypress-io/cypress into astone123/create-from-react-server
The diff is massive because of some issues that we had with V8 snapshots. The main changes are in the following directories:
You should be able to collapse the rest of the changes and trust CI for those |
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.
Very cool stuff 😎. Couple edge cases identified that will need to be addressed.
packages/data-context/test/unit/actions/project/counter-multiple-components.jsx
Show resolved
Hide resolved
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.
Thanks for adding the new test cases. Looks like we're leaving the "non-function return type" resolver for future work, along with potentially splitting the codegen actions into a separate *Actions
container. Works for me
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.
There is another edge case with regards to export { Foo as Bar }
but will add that to #24929
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, couple of quality-of-life changes I think are worth doing. LMK what you think.
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.
Looking good to me. Merge when ✔️ on CI!
* feat: server logic for create from React component (#24881) Co-authored-by: Ryan Manuel <[email protected]> Co-authored-by: Lachlan Miller <[email protected]> * fix: add default export detection (#24954) Co-authored-by: astone123 <[email protected]> * update cache * update yarn.lock to fix builds * fix: compilation with webpack preprocessor * feat: create from React component UI (#24982) * feat: WIP server logic for create from React component * feat: add more tests; error handling * feat: WIP create from React UI * feat: PR feedback [run CI] * feat: try committing snapshot cache changes [run ci] * feat: try re-generating snapshot [run ci] * fix build * regenerate cache on darwin * update caches * Revert "feat: try re-generating snapshot [run ci]" This reverts commit d763e1f. * fix typing error * types * fix test * chore: try using [email protected] * update test * regen linux snapshot * update snapshots for darwin * re-gen linux snapshot * yarn install * update snapshots * update snapshot metadata * update snapshots due to babel deps changing slightly * make react docgen a dep * update tests * revert * snapshots again?? * revert * update * update * try change snapshot * change snap * update snap * feat: remove unnecessary ts-ignore * feat: add more test cases * feat: create CodegenActions; other minor refactors * feat: continue UI work * feat: ignore config and Cypress-related files * feat: PR feedback * update Vue component link * merge in default export work * consolidate graphql queries * other misc feedback * use network-only policy to fetch files; include cypress/ dir for code gen candidates; fix type error * add basic e2e test * fix app integration tests * refactor and fix app component and webpack dev server tests * add error state; fix unit tests [skip ci] * simplify generator show logic [skip ci] * more testing * fix types * style updates [skip ci] * fix error state [skip ci] * fix list padding [skip ci] * use slots (#25079) * add more tests; fix unit tests * fix types * fix test describe * add percy snapshots for new list * update trouble rendering banner link [skip ci] * use collapsible component * use button for component list items * fix tests * build binaries * revert changes to circle config * remove eslintignore and extra loading div [skip ci] because we know it will fail * revert changes to framework glob patterns [skip ci] Co-authored-by: Ryan Manuel <[email protected]> Co-authored-by: Lachlan Miller <[email protected]> * feat: pass parser options to allow parsing of tsx files (#25145) * fix create from component e2e test * build binaries [run ci] * fix component tests [run ci] * regen windows snapshot Co-authored-by: Ryan Manuel <[email protected]> Co-authored-by: Lachlan Miller <[email protected]> Co-authored-by: Zachary Williams <[email protected]>
NOTE: This targeting a feature branch
feature/create-from-react-component
.User facing changelog
n/a
Additional details
NOTE: The work in this PR assumes that the component passed to
spec-options
is a named export. In #24929 we will add support for components that are exported by default.This PR does the following:
getReactComponentsFromFile
that the front end can call to parse a JS/TS file and get info about the exported React components that are defined in the filereact-component
spec templatespec-options
to accept acomponentName
option and to generate a spec using thereact-component
template when the project framework is ReactSteps to test
There aren't many ways to manually test this without a front end. Take a look at the automated tests that I've written so far.
How has the user experience changed?
n/a
PR Tasks
cypress-documentation
?type definitions
?