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

Update React template with ESM and type #152

Closed
wants to merge 1 commit into from
Closed

Update React template with ESM and type #152

wants to merge 1 commit into from

Conversation

karlhorky
Copy link
Contributor

@karlhorky karlhorky commented Jun 19, 2019

Hey, thanks for this project, really great work!

Because we have compilerOptions.esModuleInterop set to true in tsconfig.json, we can add the normal ES Modules syntax for importing React, or am I missing something?

And we can use the React.FC type for the dummy component.

@TrySound
Copy link
Collaborator

import * as React from 'react'; is normal es modules syntax too. Actually it will be the only way to import react in a few years. Timing depends on how much users will adopt it.

@karlhorky
Copy link
Contributor Author

Actually it will be the only way to import react in a few years

Hm, interesting to hear this, what's your source? Haven't seen anything pointing to that in the ecosystem...

But that notwithstanding, the idiomatic way to import React currently in the official documentation and any non-TypeScript tutorial is currently import React from 'react'; so it seems like we should stick with this for now.

@swyxio
Copy link
Collaborator

swyxio commented Jun 19, 2019

🤷 not something i care too much about, anyone using tsdx will well be able to tweak that file on their own.

@jaredpalmer
Copy link
Owner

Gonna close as this is a project-by-project decision.

@jaredpalmer
Copy link
Owner

Also, IIRC from Formik if you do this in your lib, you force ppl consuming it to do it as well

@karlhorky karlhorky deleted the patch-1 branch June 21, 2019 09:29
@karlhorky
Copy link
Contributor Author

anyone using tsdx will well be able to tweak that file on their own
project-by-project decision

Both true.

However I would argue that since you are making a zero-config scaffolder for new projects, you are in the unique position of teaching people that are new to a language or framework what the common defaults are in the ecosystem with your choices of default templates. Some users (both beginners and not) will want and place weight in the suggestion from you, and starting with a suggestion that doesn't match the majority of React teaching materials is confusing.

As a further reference, scaffolding out a TS app with create-react-app does the thing that I did in this PR.

IIRC from Formik if you do this in your lib, you force ppl consuming it to do it as well

This would potentially be an issue, do you have a reference for this?

@TrySound
Copy link
Collaborator

Here's my source facebook/react#11503
Named exports make more sense than single default export. It can be optimised more carefully. So think of it as a recommendation from this project.

@karlhorky
Copy link
Contributor Author

karlhorky commented Jun 21, 2019

Hm, this is all news to me, so thanks for the link.

However, I think your interpretation of the sentiment is different than mine - the discussion in that thread isn't exactly ringing praise for this syntax:

Should we encourage people to import * for better tree shaking?

I guess this is not too bad, even if a bit odd:

And from your facebook/react#13080 PR:

Wouldn't we want to support import React from 'react' in an ESM environment too? While not ideal, not doing this could be pretty bad for all the tutorials and examples out there.

The last part is what I'm focusing on.

It may be better in the future to change this syntax in tutorials, examples and tooling like scaffolders, but that change should probably be done then, when (and if) it ever becomes standard/idiomatic.

@jaredpalmer
Copy link
Owner

tsdx shouldn’t make this decision tho. Your app should. If you use this, AFAIK, then you force this decision onto all users of your library. This happened once in Formik and people got pissed off because it broke their stuff.

@karlhorky
Copy link
Contributor Author

Either way it is a decision made by tsdx and recommended to users, also currently.

you force this decision onto all users of your library

I agree this is an unacceptable problem, and would like to know more about it. Maybe there's a way around it. And if not, maybe it should be reported as a bug.

@karlhorky
Copy link
Contributor Author

karlhorky commented Jun 22, 2019

Was jaredpalmer/formik#625 the issue in Formik?

I tried searching for import *, but didn't find much else that sounded like what you're describing:

https://github.com/jaredpalmer/formik/issues?page=1&q=is%3Aissue+is%3Aclosed+import+%2A&utf8=%E2%9C%93

@karlhorky
Copy link
Contributor Author

Re: @TrySound's comment, looks like there's been some more movement in this direction from Facebook (PR Sebastian Markbåge):

facebook/react#18102

Maybe import * as React from "react"; will start seeing some more uptake...

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.

4 participants