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

Add react-refresh-typescript to documentation #248

Merged
merged 6 commits into from
Apr 14, 2021
Merged

Add react-refresh-typescript to documentation #248

merged 6 commits into from
Apr 14, 2021

Conversation

Jack-Works
Copy link
Contributor

@Jack-Works Jack-Works commented Nov 8, 2020

close #211, close #34

@pmmmwh
Copy link
Owner

pmmmwh commented Nov 10, 2020

Hey - thanks for the PR! I'm open to this change, but I want to further formulate the narrative a bit that this is not officially maintained by the React team (not to disregard your work, but for some people who work in corps or cares about being purely React they would need that information) - can you add that in?

Copy link
Owner

@pmmmwh pmmmwh left a comment

Choose a reason for hiding this comment

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

LGTM - can you please (if you haven't) test run the example and make sure that it works, and also would recover from errors?

(I'll do it when I merge the PR but just want to be double sure)

@Jack-Works
Copy link
Contributor Author

I've tried hmr works but not error recovery

@pmmmwh
Copy link
Owner

pmmmwh commented Nov 18, 2020

I'll further test this out a bit now that we have all the tests back up 😃

@pmmmwh pmmmwh self-requested a review November 18, 2020 14:29
@Jack-Works
Copy link
Contributor Author

hi, any progress?

@pmmmwh
Copy link
Owner

pmmmwh commented Dec 31, 2020

Setting up tests to run for this scenario now - if they all pass I'll merge this in.

Copy link

@hg-pyun hg-pyun left a comment

Choose a reason for hiding this comment

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

LGTM. This is the solution I was looking for.

README.md Outdated Show resolved Hide resolved
@Jack-Works
Copy link
Contributor Author

rebased and resolve reviews

@hg-pyun
Copy link

hg-pyun commented Jan 10, 2021

@pmmmwh Please Review 👍

@yoruvo
Copy link

yoruvo commented Jan 16, 2021

Hi,

just dropping in to say that this approach fixed my problem of being unable to use this plugin with TypeScript.

My single gripe is that you need to require("react-refresh-typescript").default instead of just require("react-refresh-typescript"). That seems to oppose the way other plugins are written. This is obviously a very minor gripe though.

Also, the readme change doesn't seem to mention that CommonJS isn't supported as tsconfig target.

@Jack-Works
Copy link
Contributor Author

@yoruvo I wrote no commonjs support in my repo maybe I should add it here too. Nice catch, generally you don't need to feed commonJS to webpack so you can overwrite your settings in your ts-loader

@Jack-Works
Copy link
Contributor Author

@yoruvo In 1.1.1 you can require it directly without .default

@hg-pyun
Copy link

hg-pyun commented Feb 9, 2021

@Jack-Works @pmmmwh Is it ready?

@Jack-Works
Copy link
Contributor Author

It's production ready in our app. You can install and use it without this PR, this is just a document PR

@pmmmwh
Copy link
Owner

pmmmwh commented Feb 9, 2021

Hi, I'm very sorry that I have been dragging this for such a long time.

I have a partially working test setup for this and I really want this to pass all conformance tests before officially recommending it. I'll see if I can whip up a branch to track the status of that this week.

@pmmmwh pmmmwh changed the title Add react-refresh-typescript Add react-refresh-typescript to documentation Feb 9, 2021
@pmmmwh pmmmwh merged commit baf0103 into pmmmwh:main Apr 14, 2021
@Jack-Works Jack-Works deleted the patch-1 branch April 15, 2021 00:29
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.

Support TypeScript loader! Support typescript loader ?
5 participants