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

RFC 256: ReactCDK - Add JSX/TSX Support #258

Closed
wants to merge 8 commits into from
Closed

Conversation

pfeilbr
Copy link

@pfeilbr pfeilbr commented Sep 27, 2020


By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license

@pfeilbr pfeilbr changed the title RFC: 256 ReactCDK: Add JSX/TSX Support RFC 256: ReactCDK: Add JSX/TSX Support Sep 27, 2020
@pfeilbr pfeilbr changed the title RFC 256: ReactCDK: Add JSX/TSX Support RFC 256: ReactCDK - Add JSX/TSX Support Sep 27, 2020
@pfeilbr pfeilbr mentioned this pull request Sep 27, 2020
7 tasks
eladb
eladb previously requested changes Oct 5, 2020
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

How do we determine the construct IDs? It seems like they are implicitly determined somehow but I haven't seen any discussion about this.

I think a prototype is in order in order to flush out the details.

pfeilbr and others added 2 commits January 11, 2021 11:31
add end quote

Co-authored-by: Andrew Luca <thendrluca@gmail.com>
Updated to function.

Co-authored-by: Andrew Luca <thendrluca@gmail.com>
@mergify mergify bot dismissed eladb’s stale review January 11, 2021 16:35

Pull request has been modified.

@pfeilbr
Copy link
Author

pfeilbr commented Jan 11, 2021

Thanks @iamandrewluca for the fixes.

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

I think from a design perspective we should aim for this to support any CDK (cdktf/cdk8s) and not just the AWS CDK.


ReactCDK.render(MyApp, {region: 'us-east-1'})
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add to the README what would be the next steps users need to take in order to synth and deploy this app?

# Drawbacks

* React is not universally liked. If it becomes associated with CDK there is potential brand/product damage.
* CDK is associated and tied to AWS brand. AWS tends to support all "popular" frameworks equally. Favoring one might
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's not a concern. Having support for multiple languages and idioms is actually a tenet of the CDK, and as such this proposal enhances the ecosystem.

* mapping from react concepts of props, state, children, render props to typescript
CDK constructors, params, hierarchical nesting (parent), etc.
* code generated based on the jsii manifests of CDK modules
[via]((https://twitter.com/emeshbi/status/1305017904027643906?s=20)) @eladb
Copy link
Contributor

Choose a reason for hiding this comment

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

This is possible but probably the hardest part. Constructs often offer imperative APIs that won't be expressive JSX but can technically still be accessible from code.

The work we did on decdk can be leveraged.

const { Api, Resource, Integration } = require('@aws-cdk/react/apigateway')
const Lambda = require('@aws-cdk/react/lambda')

const EchoLambda = (

Choose a reason for hiding this comment

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

Suggested change
const EchoLambda = (
const EchoLambda = () => (

</Lambda>
)

const EchoApi = (

Choose a reason for hiding this comment

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

Suggested change
const EchoApi = (
const EchoApi = () => (

</CloudFrontDistribution>
)

const MyApp = (

Choose a reason for hiding this comment

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

Suggested change
const MyApp = (
const MyApp = () => (

</App>
)

ReactCDK.render(MyApp, {region: 'us-east-1'})

Choose a reason for hiding this comment

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

Suggested change
ReactCDK.render(MyApp, {region: 'us-east-1'})
ReactCDK.render(<MyApp />, {region: 'us-east-1'})

Comment on lines +32 to +38
async ({event, context}) => ({
"statusCode": 200,
"headers": {
"Content-Type": "application/json"
}
"body": JSON.stringify(event)
})

Choose a reason for hiding this comment

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

In my opinion the code for a lambda should still be a separated file, and be passed to <Lambda /> as a prop as a path to the code

e.g.

<Lambda handler="src/handler" />

@iamandrewluca
Copy link

iamandrewluca commented Jan 12, 2021

I see 2 ways how this can be done.

  • JSX is transformed into direct calls to CDK constructs
  • JSX istransformed directly into CloudFormation (yml/json) format

Creating components for all existing constructs/api, should involve a loooot of work.
Using one of solutions above, should be easier and more straight forward.

And in the end if JSX is transformed to something like constructs, then jsii can also be used, from my understanding.

ps: I'm new to AWS CDK, I don't know exactly what I'm doing 😆 how it works under the hood 🙂

@iamandrewluca
Copy link

iamandrewluca commented Jan 22, 2021

So we need to create a JSX transformer that respects https://github.com/aws/constructs interface
And should work for cdktf/cdk8s because they also respect constructs interface

@nija-at
Copy link
Contributor

nija-at commented Jul 20, 2021

RFC Bar Raiser: eladb@

@nija-at nija-at assigned eladb and unassigned pfeilbr Jul 20, 2021
@eladb
Copy link
Contributor

eladb commented Aug 1, 2021

@pfeilbr I am closing this for now. Please reach out if you are interested to continue working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants