add hints to troubleshoot NextJS + TS issues.#2703
add hints to troubleshoot NextJS + TS issues.#2703justincorrigible wants to merge 1 commit intoemotion-js:mainfrom justincorrigible:patch-1
Conversation
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit dd30f9f:
|
srmagura
left a comment
srmagura
left a comment
There was a problem hiding this comment.
Thanks for the PR. I am open to improvements to this docs page, but I don't want to recommend @jsx jsx at the top of every file unless that is really the only way.
| ### With the old JSX transform | ||
|
|
||
| If you are unable to upgrade to the `react-jsx` transform, you will need to specify the JSX factory at the top of every file: | ||
| If you are unable to upgrade to the `react-jsx` transform (e.g. while using NextJS, which enforces `"jsx": "preserve"`), you may need to specify the JSX factory at the top of every file: |
There was a problem hiding this comment.
@jsx jsx at the top of every file is not necessary to get Next, TypeScript, and Emotion to play nicely together, so I don't think we should recommend this to users. I rewrote the Emotion documentation site in Next & TypeScript (#2571) and got it to work with jsxImportSource in tsconfig.json and @emotion/babel-plugin — though I'm pretty sure the Babel plugin is not truly necessary here.
There was a problem hiding this comment.
thank you!
I also find that instruction to "add this everywhere" a bit odd, and wholeheartedly agree with you about it... however, as you may realise, it is not part of my changes. As you will notice in the existing docs, they already had that statement before my PR.
In fact, I'm suggesting to modify "will need" to "may need", so it doesn't sound as mandatory.
I tried to change as little as possible in the existing verbiage, but you're right: that line needn't be added everywhere, and I'd like to improve that too.
Naturally, I'm willing to improve this PR if you can advise better phrasing for those other sentences/words I didn't add/change. Any suggestions? I'm also ok closing this PR if your rewrite handles the issue I had.
There was a problem hiding this comment.
side note, you mention you got it to work with just the babel plugin and jsxImportSource? for some reason I wasn't able to get css props going correctly until I also added the /// declaration.
will give your rewrite a good read, and maybe you'll let me pick your brain if I cannot manage to make it work that way?
edit: just went through your rewrite (awesome amounts of work, btw), and could not find any other differences in config, apart from the NextJS version itself (I'm on ^12.1.2). I wonder if that (or any other package's version difference) is what didn't let the css props work? 🤷
Additionally, I see you're removing the pragma from all the docs, but I'm not sure I recall what you will recommend for people not using babel (or the emotion babel plugin, for that matter)?
There was a problem hiding this comment.
Thank you @caravinci 😊
- The reason I commented on your change to this line is because, by mentioning Next.js in this way, it sounds like we are recommending
@jsx jsxfor all Next.js users. - I'm not sure why you needed the
///declaration to get TypeScript to recognize thecssprop. I believejsxImportSourceintsconfig.jsonis all that's needed to tell TypeScript about the extra prop. - I removed the
@jsxpragma from all of the code samples because, some of those are live examples and the pragma was messing them up, and also I want the docs to be focused on thereact-jsxtransform which does not require the pragma. - I think the Emotion docs could be improved by adding an "Integration Guides" heading to the navigation menu on the right. It would be below the "Advanced" heading. The nav items under the new heading would be "Create React App / CodeSandbox", "Next.js", .etc. If you would like to work on this, I can create a new issue and reach out to Andarist for approval.
There was a problem hiding this comment.
The reason I commented on your change to this line is because, by mentioning Next.js in this way, it sounds like we are recommending @jsx jsx for all Next.js users.
Though I'm not sure I would read it that way, I see what you mean and how others may indeed read it that way too. Those not using babel, in particular, may be led to misunderstand my changes.
Your comment makes sense to me now. Thanks for pointing that out 🙏
jsxImportSource in tsconfig.json is all that's needed to tell TypeScript about the extra prop
yeah, it seems reasonable to me too, but for some reason it didn't work in my project/setup. There may be something else going on here that's not obvious to me right now. I'll run some tests this weekend, see if I can at least figure out what the root issue is, and document it.
the Emotion docs could be improved by adding an "Integration Guides"
oh, that would be awesome, and very useful... especially as the NextJS team are actively working on better integration for their SWC. I will gladly help if possible, pending availability at my job, of course.
That said, this PR is not necessary.
|
Closing this PR in favour of an eventual new "integration" section in the docs. |
2 participants
Added details to the TypeScript documentation, geared towards NextJS users.
Making these changing because of the way NextJS enforces its configs, which led me to find this workaround.
AFAIK, there's no mention of NextJS in these instructions, and adding keywords here and there may help others find the solution like I did.
hm not sure how to answer this. sorry.
TestsN/ACode completeN/AChangesetN/AThanks for this amazing tool!