-
Notifications
You must be signed in to change notification settings - Fork 26.5k
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
Updated Typescript example and config files #1610
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
links in PR description don't work.
"typescript": "^2.1.5" | ||
"typescript": "2.2.1", | ||
"react":"latest", | ||
"react-dom":"latest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
latest react seems unsafe. i'd pick a version range just like above. how about ^15.4.0
?
also space after the colon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx
@@ -1,8 +1,8 @@ | |||
import * as React from 'react' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my experience every TS file that uses JSX must have this exact line, because the JSX definitions are defined in the react types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
roger :)
<MyComponent /> | ||
</div> | ||
<Link prefetch href='/pagewithasync'><a>Reactpage</a></Link> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you really supposed to put an anchor tag inside a Link
? seems like the Link
is the anchor tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to the way proposed in readme
} | ||
constructor() { | ||
super(); | ||
this.state = { clicked: false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only set state in constructor if it relies on props. this should be a simple member.
public state: pageState = { clicked: false };
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - I am learning from this :)
this.setState({ clicked: true }) | ||
} | ||
render() { | ||
return <div onClick={this.click.bind(this)}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoa no this is no good. do the bind on the function definition itself.
private click = () => { /* bound */ }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started doing that, but didn't like the way it compiled - but I'll do it the right way again now. Thx
"jsx": "react", | ||
"allowJs": true | ||
"module": "es2015", | ||
"target": "es2017", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you changing these two fields? this is a pretty significant shift in how the example works. what's wrong with commonjs es6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the pull request comment
"moduleResolution": "node", | ||
"lib": [ | ||
"dom", | ||
"es2017" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed? async/await
are supported natively in ts 2.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See update PR comment above
interface pageState { | ||
clicked: boolean | ||
} | ||
export default class extends React.Component<pagesProps, pageState> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i prefer whitespace between every block (blank line after every }
). give your code some room to breathe man!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:) Now it's breathing
interface pagesProps { | ||
stars: number | ||
} | ||
interface pageState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interface names should use PascalCase just like classes. i prefer the I
prefix convention too as it makes it obvious that this is a TS-only construct (and cannot be used in JS code): IPageProps
, IPageState
.
refer to any Blueprint component (perhaps this one) for tried-and-true conventions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To use options in the config to be able to get a readable output to be able to post on SO makes absolutely no sense to me. It doesn't take longer to remove the TS syntax sugar than making the code postable (making the code more generic, remove irrelevant stuff, remove customer specific stuff etc). If you post the output then you might end up with code that works and can be readable but still makes no sense to a human in the way it's being written. And if you want to implement an answer you get you will have to do some reverse engineering and figure out how tsc transpiles the code and how you should implement it in TS to get the same output as the answer. This feels like one of those "It works, but..."-things.
@@ -0,0 +1,28 @@ | |||
import * as React from 'react' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe consider camelCasing in the filename?
const res = await fetch('https://api.github.com/repos/zeit/next.js'); | ||
const data = await res.json(); | ||
return { stars: data.stargazers_count } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe some space after these }
as well @giladgray?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import` React from 'react'
import 'isomorphic-fetch'
export default class extends React.Component {
static async getInitialProps () {
const res = await fetch('https://api.company.com/user/123')
const data = await res.json()
return { username: data.profile.username }
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes please blank line after every function, class, member, etc.
a little tslint
goes a long way.
I think the question is why would you want Typescript to compile it in the first place? |
I agree but three out of four of the Advantages listed in the PR is about posting code on SO and Issues and being able to actually use the output instead of the TS files. |
@OskarKlintrot isn't it 2/4? |
To be honest, I appreciate the fix to get the example up and running but I don't really see the point of everything else (except maybe Something that I personally think would contribute to the project is a "production ready"-example with proper setup of TS and handling of assets, the build in a separate folder (not the root), unit testing etc. Something to actually build upon to get something that can be delivered to a customer but still providing great DX. Basically having two examples:
As I said I'm not part of Next.js and I'm currently using dotnet core for SSR and TS so don't take my words too serious, but... I think it can be a good idea to open up a new issue to either a) discuss rewriting the example (as you have done here) if you really really want that or b) discuss adding a new example (as I written above). See what the members and community of Next.js think and go from there. I understand that it must be frustrating to code stuff that just gets criticized and rejected so having a clear picture of what the project wants and needs is probably a good way forward. As I have written before, we discussed the current example for three weeks before everyone was satisfied and that's not a lot code and very unopinionated! |
Ping @giacomorebonato who wrote the current example. |
Yeah but that's like updating
Again, I'm not part of Next.js and don't talk for them. This is just a common way of contributing to OS projects. |
Keystonejs is an awesome project with some really solid guidelines for contributing that might be worth looking at. Their workflow is pretty much what I have described above. |
'Yeah but that's like updating package.json and changing two lines in tsconfig.json.' +3 |
I start to feel like I'm repeating myself but... I know that you haven't done a more complete example but you still have rewritten the example that we spent three weeks working on and that is where my problem is with this approach. I'm not complaining about necessary changes in To sum up my thoughts on this:
Again, just MHO's. |
You and me both (about the repeating ;-) But I think we are on the same page (or close) I have some work now - I hope others will try my version and comment. I am learning from this :) |
Work was maybe the wrong word, discussion is what I meant but you get the point. |
I like the "jsx": "react-native" settings, so |
@giacomorebonato '..to ES6 so I can use latest language feature..' I think you can do that regardless. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one of my favorite things about typescript is that it's basically babel + types (i never use babel in my typescript projects because tsc
handles everything i could want out of the box, and then some). but Next itself provides support for a handful of ES6 concepts (namely async/await and JSX) so it kinda makes sense in this case to not handle those in tsc
.
however, caring about the readability of the compiled output is the absolute wrong approach: typescript is a compiler, and you should trust it to produce functional code. you should not, however, expect to read or share that compiled code and it's unreasonable to hamstring your project for that goal.
const res = await fetch('https://api.github.com/repos/zeit/next.js'); | ||
const data = await res.json(); | ||
return { stars: data.stargazers_count } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes please blank line after every function, class, member, etc.
a little tslint
goes a long way.
Some may (rightfully) care about lock in and bloating the code. |
@giacomorebonato Any thoughts on |
If I target ES5 with TypeScript^2.2.2, I can't use even the Promise keyword. @bestwestern Do you confirm? @OskarKlintrot "preserve" would be semantically more correct, but the problem when I made the PR was that it compiled to |
Thanks for the explanation @giacomorebonato! 👍 |
I am receiving a lot of `Cannot use JSX unless '--jsx' flag is enabled' and alias doesn't work with this configuration. |
@leesiongchan are you writing JSX in .tsx files? The configuration should work then. |
I'm going to close this in favor of #2391 |
I added props and state to the example the way Typescript recommends
https://www.typescriptlang.org/docs/handbook/react-&-webpack.html
I updated tsconfig so the compiled code becomes what Zeit suggest at (e.g. async await)
https://zeit.co/blog/next
This way Typescript does it thing (intellisense, warnings at compiletime and removes declarations) and Zeit does the rest. (in the old example Typescript compiled to js and not jsx)
Advantages:
The updates to the tsconfig are all necessary to achieve this.