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

docs(guides): typescript-guide rewrite #1446

Merged
merged 3 commits into from
Jul 30, 2017

Conversation

mtrivera
Copy link
Contributor

Loader

Removed awesome-ts-loader and moved it to bottom of guide. I think it’s confusing to list two loaders if one is preferred for simplicity, focus on the one that is simple to use.

Source Maps

Some people might not know about source maps, so mention another guide that discusses it.

Using Third Party Libraries

The word third is easier to read than 3rd, and people might not be familiar with its definition. I replaced types github repo with TypeSearch. The search functionality on TypeSearch is easier than reading the documentation on the DefinitelyTyped repo.

Feedback welcome and appreciated!

Copy link
Collaborator

@skipjack skipjack left a comment

Choose a reason for hiding this comment

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

Nice work @mtrivera, just left a few minor comments re formatting. Also, I was under the impression that we'd be somehow synchronizing this with the other guides (probably modifying the final example from Getting Started would work). This is what we've done for the others in #1258.

@TheDutchCoder and I could probably help with direction a bit -- this synchronization could also happen in another PR.



## Source Maps

To learn more about Source Maps, see the [development guide](/guides/development.md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing period at the end of this sentence.

@@ -136,3 +134,10 @@ declare module "*.svg" {
```

Here we declare a new module for SVGs by specifying any import that ends in `.svg` and defining the module's `content` as `any`. We could be more explicit about it being a url by defining the type as string. The same concept applies to other assets including CSS, SCSS, JSON and more.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We usually use two line breaks between h2 sections.

[`awesome-typescript-loader`](https://github.com/s-panferov/awesome-typescript-loader)

Awesome TypeScript Loader has created a [wonderful explanation](https://github.com/s-panferov/awesome-typescript-loader#differences-between-ts-loader) of the difference between `awesome-typescript-loader` and `ts-loader`. The
configuration for `awesome-typescript-loader` is more complex than `ts-loader`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should just be joined with the previous line. At some point we may enforce a max line length (webpack-contrib/webpack-defaults#73) or something to keep it consistent but for now each paragraph should just be on one line for consistency. Then you can turn on word-wrapping in your editor if it's a pain to read.

Copy link
Collaborator

@TheDutchCoder TheDutchCoder left a comment

Choose a reason for hiding this comment

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

Apart from the comments that @skipjack made, this looks good to me.

We can align it with the other guides at a later stage, or, if @mtrivera feels like it, he can tackle it in this one.

I have no experience with TS, so it might be challenging for me :)

@mtrivera
Copy link
Contributor Author

I made the corrections

Copy link
Collaborator

@skipjack skipjack left a comment

Choose a reason for hiding this comment

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

@mtrivera great thank you!

@skipjack skipjack merged commit 8daac33 into webpack:master Jul 30, 2017
@skipjack
Copy link
Collaborator

@mtrivera are you interested in synchronizing this with the other guides in a separate PR?

@mtrivera
Copy link
Contributor Author

mtrivera commented Aug 1, 2017

Sure, but I would need some help with that.
Are you referring to this from a couple of days ago?

Also, I was under the impression that we'd be somehow synchronizing this with the other guides (probably modifying the final example from Getting Started would work). This is what we've done for the others in #1258.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants