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

Server compat should have a default export #4047

Merged
merged 1 commit into from
Jun 24, 2023

Conversation

MikaFima
Copy link
Contributor

Hi preact,

For now the use of preact/compat do not allow to use import ReactDOM from 'react-dom/server', as it will fail with an error:
image

This PR is here to allow it, without breaking the classical use import {renderToStaticMarkup} from 'react-dom/server'

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

I'm a bit confused by the changes proposed in this PR. From what I can tell they are mostly stylistic changes and don't change the way a the code is exported at all.

@MikaFima
Copy link
Contributor Author

Ok, as you can see with your client.js https://github.com/preactjs/preact/blob/master/compat/client.mjs, you need a default export to be imported with import ReactDOM from 'react-dom/server'.

Do you prefer I add some tests to make it sure both imports work as expected ?

@MikaFima
Copy link
Contributor Author

ANother example, i'm working with draft-convert and there is this kind of error too:

image

In order to fix this, I need a default export

compat/server.browser.js Outdated Show resolved Hide resolved
compat/server.js Outdated Show resolved Hide resolved
@MikaFima MikaFima force-pushed the compat-server-default-export branch from 348d5f2 to 65f73f8 Compare June 16, 2023 15:33
@MikaFima
Copy link
Contributor Author

Is that okay for you ?

@@ -1,4 +1,11 @@
import { renderToString as preactRenderToString } from 'preact-render-to-string';
Copy link
Member

Choose a reason for hiding this comment

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

nit: No need for the as, just keep it as renderToString

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -1,4 +1,11 @@
import { renderToString as preactRenderToString } from 'preact-render-to-string';
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@MikaFima MikaFima force-pushed the compat-server-default-export branch from 65f73f8 to 90d618c Compare June 22, 2023 10:23
@coveralls
Copy link

Coverage Status

coverage: 99.706%. remained the same when pulling 90d618c on MikaFima:compat-server-default-export into 523bbe9 on preactjs:master.

@JoviDeCroock JoviDeCroock merged commit 56d8f41 into preactjs:master Jun 24, 2023
@JoviDeCroock JoviDeCroock mentioned this pull request Jul 8, 2023
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.

5 participants