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(API) Add error report introduction of loaders (#2231) #2298

Merged
merged 15 commits into from
Dec 16, 2018

Conversation

mc-zone
Copy link
Member

@mc-zone mc-zone commented Jun 26, 2018

These additions resolve #2231 .

@mc-zone
Copy link
Member Author

mc-zone commented Jun 26, 2018

[email protected] lint:markdown /home/travis/build/webpack/webpack.js.org
markdownlint --config ./.markdownlint.json .md ./src/content/**/.md
./src/content/api/loaders.md: 355: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 357: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 361: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 366: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 373: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 375: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 382: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 384: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 391: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 394: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 401: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 403: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 410: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 412: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 419: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 421: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 438: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 440: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 447: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 449: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 512: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 514: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 518: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 522: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 526: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 542: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 566: MD031 Fenced code blocks should be surrounded by blank lines
./src/content/api/loaders.md: 572: MD031 Fenced code blocks should be surrounded by blank linesnpm ERR! code ELIFECYCLE

I don't know whether I should fix this. but the CI test can't pass if I don't.

@montogeek
Copy link
Member

@mc-zone Yes, you should, It was passing before.

@dhruvdutt
Copy link
Member

@mc-zone Not sure why exactly it's failing but take a look at these rules.

@mc-zone
Copy link
Member Author

mc-zone commented Jun 27, 2018

[email protected] fetch /home/travis/build/webpack/webpack.js.org
sh src/scripts/fetch.sh
{ [Error: {"message":"API rate limit exceeded for 35.184.48.144. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)","documentation_url":"https://developer.github.com/v3/#rate-limiting"}]
message: '{"message":"API rate limit exceeded for 35.184.48.144. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)","documentation_url":"https://developer.github.com/v3/#rate-limiting"}',
code: 403,
status: 'Forbidden',

Seems we have to fetch some content every time. Can these be cached or some other workaround? @montogeek

@montogeek
Copy link
Member

@mc-zone I have tried, twice (#2270 , #1963), will try again

@mc-zone
Copy link
Member Author

mc-zone commented Jul 9, 2018

Anyone could review this? is this enough to be merged? 🚶

Copy link
Member

@montogeek montogeek left a comment

Choose a reason for hiding this comment

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

Nice additions!


### Using `throw` (or other uncaught exception)

Throw out an uncaught error while loader are running will cause current module compiling failed.
Copy link
Member

Choose a reason for hiding this comment

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

...while a loader is running is correct.
Correct word is fail.

Copy link
Member

Choose a reason for hiding this comment

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

or: Throwing an error while loader is running will cause current module compilation failure.


```

the module that ouccur this error will be packed into bundle (as an error module) like the following:
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what you mean on this line

Copy link
Member

Choose a reason for hiding this comment

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

The module on which loader had thrown this error will get bundled like this:


```

And you can see these informations below, not only error message, but also details about which loader and module are involved:
Copy link
Member

Choose a reason for hiding this comment

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

As you can see below would sound better

Copy link
Member

Choose a reason for hiding this comment

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

For sure, shorter and clearer


## Error Reporting

There are some ways to throw out errors from inside a loader:
Copy link
Member

Choose a reason for hiding this comment

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

this sounds abstract, please be more specific. e.g. Here are the ways to throw an error from inside a loader:...


### Using `this.emitError`

It just report errors but not interrupt module compiling.
Copy link
Member

Choose a reason for hiding this comment

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

[this.emitError](/api/loaders/#this-emiterror) will report the errors without interrupting module's compilation. or any other way you can rephrase this to sound closer to software documentation page


It just report errors but not interrupt module compiling.

see [this.emitError](/api/loaders/#this-emiterror).
Copy link
Member

Choose a reason for hiding this comment

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

see above

- the loader path: `(from ./src/loader.js)`
- the caller path: `@ ./src/index.js 1:0-25`

W> The loader path about error in the above are added after webpack 4.12
Copy link
Member

Choose a reason for hiding this comment

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

The loader path in the error is displayed since webpack 4.12

Copy link
Member

@EugeneHlushko EugeneHlushko left a comment

Choose a reason for hiding this comment

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

I would really love to see texts shaped accordingly with the rest of documentation. Right now they are not of a documentation level

@montogeek
Copy link
Member

@mc-zone Friendly ping.

@mc-zone
Copy link
Member Author

mc-zone commented Aug 14, 2018

Updated. @montogeek @EugeneHlushko Sorry about this...

Copy link
Member

@EugeneHlushko EugeneHlushko left a comment

Choose a reason for hiding this comment

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

change compiling to compilation to be consistent

src/content/api/loaders.md Outdated Show resolved Hide resolved
src/content/api/loaders.md Outdated Show resolved Hide resolved
src/content/api/loaders.md Outdated Show resolved Hide resolved
@mc-zone
Copy link
Member Author

mc-zone commented Nov 23, 2018

@montogeek @EugeneHlushko Hi, is there anything else that I should do for this PR? Please tell me if anything wrong. Thanks!

Copy link
Member

@EugeneHlushko EugeneHlushko left a comment

Choose a reason for hiding this comment

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

Thank you for updating the PR! It looks really good now, a couple of small changes requested and one question re eslint

src/content/api/loaders.md Outdated Show resolved Hide resolved
src/content/api/loaders.md Outdated Show resolved Hide resolved
src/content/api/loaders.md Outdated Show resolved Hide resolved
src/content/api/loaders.md Outdated Show resolved Hide resolved
src/content/api/loaders.md Show resolved Hide resolved
Copy link
Member

@EugeneHlushko EugeneHlushko left a comment

Choose a reason for hiding this comment

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

LGTM

@EugeneHlushko
Copy link
Member

@montogeek please have another look

@montogeek montogeek closed this Dec 16, 2018
@montogeek montogeek reopened this Dec 16, 2018
@montogeek montogeek merged commit 29ce0ef into webpack:master Dec 16, 2018
@montogeek
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

Document webpack change: Add loader name to error message. Resolves #2878
5 participants