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

[HMR] Fix HMR syntax error messages (message instead of description) #17619

Closed
wants to merge 1 commit into from

Conversation

ide
Copy link
Contributor

@ide ide commented Jan 15, 2018

Motivation

The code to display HMR errors on the client was reading the description field from Metro payloads. Metro does not include description in the body of its error payloads -- only in its body.errors[] items. This commit changes RN's HMR code to show body.message (set consistently with facebook/metro#124) instead of the non-existent body.description.

Test Plan

Open a test RN app, enable HMR, and then introduce a syntax error in an app source file. See that the redbox provides information about the syntax error instead of just saying "TransformError undefined".

Related PRs

Release Notes

[GENERAL][ENHANCEMENT][HMR] - Fix display of syntax error messages when HMR is enabled

The code to display HMR errors on the client was reading the `description` field from Metro payloads. Metro does not include `description` in the body of its error payloads -- only in its `body.errors[]` items. This commit changes RN's HMR code to show `body.message` (set consistently with facebook/metro#124) instead of the non-existent `body.description`.

Test Plan: Open a test RN app, enable HMR, and then introduce a syntax error in an app source file. See that the redbox provides information about the syntax error instead of just saying "TransformError undefined".
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. cla signed labels Jan 15, 2018
@pull-bot
Copy link

@facebook-github-bot label Core Team

Generated by 🚫 dangerJS

@ide
Copy link
Contributor Author

ide commented Jan 15, 2018

@rafeca @mjesun The code in this PR hasn't been touched in over a year but since it uses Metro's error payload, Metro team members are probably the best people to look at this.

@mjesun
Copy link
Contributor

mjesun commented Jan 15, 2018

When I simplified JSTransformer/index.js in Metro here, I removed the duality between message and description, standardizing over the first because of

  • description is set to '' when it's a generic error, and
  • message is the standard one in the ECMA specification.

I then tried identifying other places where description was used, but I obviously not looked exhaustively 😅. Thanks for pointing out that one and fixing it!

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jan 16, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@mjesun is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ide added a commit to expo/react-native that referenced this pull request Jan 16, 2018
Summary:
The code to display HMR errors on the client was reading the `description` field from Metro payloads. Metro does not include `description` in the body of its error payloads -- only in its `body.errors[]` items. This commit changes RN's HMR code to show `body.message` (set consistently with facebook/metro#124) instead of the non-existent `body.description`.

Open a test RN app, enable HMR, and then introduce a syntax error in an app source file. See that the redbox provides information about the syntax error instead of just saying "TransformError undefined".

- facebook/metro#124

[GENERAL][ENHANCEMENT][HMR] - Fix display of syntax error messages when HMR is enabled
Closes facebook#17619

Differential Revision: D6726516

Pulled By: mjesun

fbshipit-source-id: b1d1008d6f1aa8f88ff8a2aa1374724a305c773b
@ide ide deleted the hmr-error-messages branch January 16, 2018 19:29
ide added a commit that referenced this pull request Jan 18, 2018
Summary:
The code to display HMR errors on the client was reading the `description` field from Metro payloads. Metro does not include `description` in the body of its error payloads -- only in its `body.errors[]` items. This commit changes RN's HMR code to show `body.message` (set consistently with facebook/metro#124) instead of the non-existent `body.description`.

Open a test RN app, enable HMR, and then introduce a syntax error in an app source file. See that the redbox provides information about the syntax error instead of just saying "TransformError undefined".

- facebook/metro#124

[GENERAL][ENHANCEMENT][HMR] - Fix display of syntax error messages when HMR is enabled
Closes #17619

Differential Revision: D6726516

Pulled By: mjesun

fbshipit-source-id: b1d1008d6f1aa8f88ff8a2aa1374724a305c773b
ide added a commit that referenced this pull request Jan 18, 2018
Summary:
The code to display HMR errors on the client was reading the `description` field from Metro payloads. Metro does not include `description` in the body of its error payloads -- only in its `body.errors[]` items. This commit changes RN's HMR code to show `body.message` (set consistently with facebook/metro#124) instead of the non-existent `body.description`.

Open a test RN app, enable HMR, and then introduce a syntax error in an app source file. See that the redbox provides information about the syntax error instead of just saying "TransformError undefined".

- facebook/metro#124

[GENERAL][ENHANCEMENT][HMR] - Fix display of syntax error messages when HMR is enabled
Closes #17619

Differential Revision: D6726516

Pulled By: mjesun

fbshipit-source-id: b1d1008d6f1aa8f88ff8a2aa1374724a305c773b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants