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

Support JSX as a pragma #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

ryota-ka
Copy link

@ryota-ka ryota-ka commented May 7, 2020

Fixes #8

Implementation of jsxFactory is just a copy and paste from @sliptype's
pull request (cyclejs/react-dom#3).
Test cases have been rewritten in a DOM-free way (using enzyme).

There are still some points that I'm not sure, so I'll leave this to be a draft.
I'll add some inline comments.

Comment on lines +1 to +3
import * as React from 'react';
import * as Adapter from 'enzyme-adapter-react-16';
import * as Enzyme from 'enzyme';
Copy link
Author

Choose a reason for hiding this comment

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

Consider turning on esModuleInterop?

@ryota-ka ryota-ka force-pushed the jsx-factory branch 2 times, most recently from 2062fa5 to be77893 Compare May 7, 2020 12:56
@@ -376,6 +376,83 @@ See [`@cycle/react-native`](https://github.com/cyclejs/react-native).
</p>
</details>

<details>
Copy link
Author

Choose a reason for hiding this comment

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

FYI: GitHub's "Display the rich diff" will help.

@ryota-ka ryota-ka changed the title Support JSX Factory Support JSX as a pragma May 7, 2020
Fixes cyclejs#8

Implementation of `jsxFactory` is just a copy and paste from @sliptype's
pull request (cyclejs/react-dom#3).
Test cases have been rewritten in a DOM-free way (using enzyme).
```js
plugins: [
new webpack.ProvidePlugin({
jsxFactory: ['@cycle/react', 'jsxFactory']
Copy link
Author

Choose a reason for hiding this comment

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

This line was originally

jsxFactory: ['react-dom', 'jsxFactory']

(see https://github.com/cyclejs/react-dom/blob/229e592ab80998ace5bf4fd0c12eee36f4499731/readme.md#automatically-providing-jsxfactory)

But I believe 'react-dom' was a mistake for '@cycle/react-dom', so now it is '@cycle/react'.

@@ -0,0 +1,36 @@
import {createElement, ReactElement, ReactType} from 'react';
Copy link
Author

@ryota-ka ryota-ka May 7, 2020

Choose a reason for hiding this comment

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

This file is a copy and paste of https://github.com/cyclejs/react-dom/blob/229e592ab80998ace5bf4fd0c12eee36f4499731/src/jsx-factory.ts, except for import path of incorporate funciton.

The original filename was kebab case (jsx-factory.tsx), but I've renamed to camel case, following the naming convention of this repository.

@@ -0,0 +1,83 @@
import * as React from 'react';
Copy link
Author

Choose a reason for hiding this comment

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

Test cases have been almost rewritten in a DOM-free way (using enzyme's shallow rendering).
The next one, however, was difficult to rewrite with my limited knowledge.

https://github.com/cyclejs/react-dom/blob/229e592ab80998ace5bf4fd0c12eee36f4499731/test/jsx-factory.tsx#L152-L185

Any good ideas?

Comment on lines +15 to +16
"jsx": "react",
"jsxFactory": "jsxFactory.createElement"
Copy link
Author

@ryota-ka ryota-ka May 7, 2020

Choose a reason for hiding this comment

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

What do you think of configuring jsx and jsxFactory here?
Or should we separately have test/tsconfig.json instead?

@staltz
Copy link
Member

staltz commented May 7, 2020

Thanks for the PR @ryota-ka!

@sliptype Could I ask your help to review this? Even a quick check would be beneficial, since you worked on this before.

@ryota-ka
Copy link
Author

ryota-ka commented Oct 11, 2020

@staltz As there's no response from @sliptype, I'll turn this PR into "Ready for review" from a draft.
Could you help me with reviewing this? Thanks!

@ryota-ka ryota-ka marked this pull request as ready for review October 11, 2020 10:23
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.

jsxFactory should be export from this package, not from @cycle/react-dom
2 participants