Skip to content

Support JSX as a pragma - #9

Open
ryota-ka wants to merge 4 commits into
cyclejs:masterfrom
ryota-ka:jsx-factory
Open

Support JSX as a pragma#9
ryota-ka wants to merge 4 commits into
cyclejs:masterfrom
ryota-ka:jsx-factory

Conversation

@ryota-ka

Copy link
Copy Markdown

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 threadtest/jsxFactory.tsx
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
Copy Markdown
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-kaforce-pushed the jsx-factory branch 2 times, most recently from 2062fa5 to be77893CompareMay 7, 2020 12:56
Comment threadreadme.md
</p>
</details>

<details>

Copy link
Copy Markdown
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-karyota-ka changed the title Support JSX FactorySupport JSX as a pragmaMay 7, 2020
ryota-ka added 2 commits May 7, 2020 21:58
Fixescyclejs#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).
Comment threadreadme.md
```js
plugins: [
new webpack.ProvidePlugin({
jsxFactory: ['@cycle/react', 'jsxFactory']

Copy link
Copy Markdown
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'.

Comment threadsrc/jsxFactory.ts
@@ -0,0 +1,36 @@
import {createElement, ReactElement, ReactType} from 'react';

@ryota-karyota-kaMay 7, 2020

Copy link
Copy Markdown
Author

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.

Comment threadtest/jsxFactory.tsx
@@ -0,0 +1,83 @@
import * as React from 'react';

Copy link
Copy Markdown
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 threadtsconfig.json
Comment on lines +15 to +16
"jsx": "react",
"jsxFactory": "jsxFactory.createElement"

@ryota-karyota-kaMay 7, 2020

Copy link
Copy Markdown
Author

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
Copy Markdown
Member

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

ryota-ka commented Oct 11, 2020

Copy link
Copy Markdown
Author

@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 freeto 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

@ryota-ka@staltz