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

improve error reporting of JSON parsing #3676

Merged
merged 3 commits into from
Feb 8, 2023

Conversation

sokra
Copy link
Member

@sokra sokra commented Feb 7, 2023

also make source context generation more reusable

This adds a source context snippet of the JSON that failed parsing and the json path to the error.

An error occurred while generating the chunk item [project]/crates/turbopack-tests/tests/snapshot/imports/json/input/invalid.json (json)
  at Execution of module_factory failed
  at Execution of JsonChunkItem::content failed
  at Unable to make a module from invalid JSON: expected `,` or `}` at line 3 column 26
  at nested.?
     1 | {
     2 |   "nested": {
       |                          v
     3 |     "this-is": "invalid" // lint-staged will remove trailing commas, so here's a comment
       |                          ^
     4 |   }
     5 | }

@vercel
Copy link

vercel bot commented Feb 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
examples-native-web 🔄 Building (Inspect) Feb 8, 2023 at 7:39AM (UTC)
examples-nonmonorepo 🔄 Building (Inspect) Feb 8, 2023 at 7:39AM (UTC)
examples-tailwind-web 🔄 Building (Inspect) Feb 8, 2023 at 7:39AM (UTC)
turbo-benchmark ❌ Failed (Inspect) Feb 8, 2023 at 7:39AM (UTC)
turbo-buildcontainer ❌ Failed (Inspect) Feb 8, 2023 at 7:39AM (UTC)
turbo-cli ❌ Failed (Inspect) Feb 8, 2023 at 7:39AM (UTC)
turbo-crates ❌ Failed (Inspect) Feb 8, 2023 at 7:39AM (UTC)
turbo-docs ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 8, 2023 at 7:39AM (UTC)
turbo-docs-at18 ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 8, 2023 at 7:39AM (UTC)
turbo-js 🔄 Building (Inspect) Feb 8, 2023 at 7:39AM (UTC)
turbo-js-q5o2 ❌ Failed (Inspect) Feb 8, 2023 at 7:39AM (UTC)
turbo-site ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 8, 2023 at 7:39AM (UTC)
turbo-tests ❌ Failed (Inspect) Feb 8, 2023 at 7:39AM (UTC)
turbo-tests-7fjh ❌ Failed (Inspect) Feb 8, 2023 at 7:39AM (UTC)
turbo-turbo-tracing-next-plugin 🔄 Building (Inspect) Feb 8, 2023 at 7:39AM (UTC)
turbo-turbo-tracing-next-plugin-ek5c ❌ Failed (Inspect) Feb 8, 2023 at 7:39AM (UTC)
turbo-with-mongodb-mongoose ❌ Failed (Inspect) Feb 8, 2023 at 7:39AM (UTC)
turbo-with-mongodb-mongoose-8o41 ❌ Failed (Inspect) Feb 8, 2023 at 7:39AM (UTC)
6 Ignored Deployments
Name Status Preview Comments Updated
examples-basic-web ⬜️ Ignored (Inspect) Feb 8, 2023 at 7:39AM (UTC)
examples-cra-web ⬜️ Ignored (Inspect) Feb 8, 2023 at 7:39AM (UTC)
examples-designsystem-docs ⬜️ Ignored (Inspect) Feb 8, 2023 at 7:39AM (UTC)
examples-kitchensink-blog ⬜️ Ignored (Inspect) Feb 8, 2023 at 7:39AM (UTC)
examples-svelte-web ⬜️ Ignored (Inspect) Feb 8, 2023 at 7:39AM (UTC)
examples-vite-web ⬜️ Ignored (Inspect) Feb 8, 2023 at 7:39AM (UTC)

@sokra sokra force-pushed the sokra/web-504-serde_json-errors-are-very-bad branch from 68a3e46 to bfc4c84 Compare February 7, 2023 18:11
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2023

⚠️ CI failed ⚠️

The following steps have failed in CI:

  • Go integration tests
  • Turbopack Rust tests (mac/win, non-blocking)

See workflow summary for details

@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2023

Benchmark for d409965

Click to view benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 9407.72µs ± 86.97µs 9579.57µs ± 100.46µs +1.83%
bench_hmr_to_commit/Turbopack RCC/1000 modules 9831.42µs ± 70.02µs 9895.46µs ± 43.46µs +0.65%
bench_hmr_to_commit/Turbopack RSC/1000 modules 493.29ms ± 1.52ms 493.48ms ± 0.80ms +0.04%
bench_hmr_to_commit/Turbopack SSR/1000 modules 9758.36µs ± 81.56µs 9920.90µs ± 105.74µs +1.67%
bench_hmr_to_eval/Turbopack CSR/1000 modules 8475.67µs ± 34.82µs 8518.19µs ± 76.04µs +0.50%
bench_hmr_to_eval/Turbopack RCC/1000 modules 8634.49µs ± 104.96µs 8735.24µs ± 59.30µs +1.17%
bench_hmr_to_eval/Turbopack SSR/1000 modules 8579.78µs ± 76.24µs 8681.31µs ± 52.58µs +1.18%
bench_hydration/Turbopack RCC/1000 modules 4229.43ms ± 6.70ms 4228.28ms ± 8.15ms -0.03%
bench_hydration/Turbopack RSC/1000 modules 3815.50ms ± 24.27ms 3851.98ms ± 18.73ms +0.96%
bench_hydration/Turbopack SSR/1000 modules 3674.27ms ± 23.96ms 3728.53ms ± 7.81ms +1.48%
bench_startup/Turbopack CSR/1000 modules 2772.87ms ± 3.58ms 2766.95ms ± 9.11ms -0.21%
bench_startup/Turbopack RCC/1000 modules 2567.04ms ± 5.54ms 2564.83ms ± 6.21ms -0.09%
bench_startup/Turbopack RSC/1000 modules 2466.64ms ± 10.73ms 2461.00ms ± 7.23ms -0.23%
bench_startup/Turbopack SSR/1000 modules 2116.41ms ± 3.29ms 2108.03ms ± 2.25ms -0.40%

@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2023

Benchmark for 23c75b8

Test Base PR % Significant %
bench_hydration/Turbopack RSC/1000 modules 3787.56ms ± 13.36ms 3857.63ms ± 20.30ms +1.85% +0.07%
bench_startup/Turbopack SSR/1000 modules 2152.07ms ± 7.28ms 2109.64ms ± 5.37ms -1.97% -0.80%
Click to view full benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 9324.12µs ± 100.99µs 9232.93µs ± 99.44µs -0.98%
bench_hmr_to_commit/Turbopack RCC/1000 modules 9000.87µs ± 64.49µs 9172.76µs ± 78.92µs +1.91%
bench_hmr_to_commit/Turbopack RSC/1000 modules 503.39ms ± 4.57ms 504.66ms ± 4.78ms +0.25%
bench_hmr_to_commit/Turbopack SSR/1000 modules 9636.40µs ± 92.80µs 9740.80µs ± 85.99µs +1.08%
bench_hmr_to_eval/Turbopack CSR/1000 modules 7811.58µs ± 67.15µs 7962.36µs ± 203.72µs +1.93%
bench_hmr_to_eval/Turbopack RCC/1000 modules 8277.68µs ± 103.55µs 8014.57µs ± 69.61µs -3.18%
bench_hmr_to_eval/Turbopack SSR/1000 modules 7915.21µs ± 45.14µs 8060.51µs ± 84.66µs +1.84%
bench_hydration/Turbopack RCC/1000 modules 4174.52ms ± 10.52ms 4198.16ms ± 7.47ms +0.57%
bench_hydration/Turbopack RSC/1000 modules 3787.56ms ± 13.36ms 3857.63ms ± 20.30ms +1.85% +0.07%
bench_hydration/Turbopack SSR/1000 modules 3711.18ms ± 7.53ms 3689.44ms ± 10.60ms -0.59%
bench_startup/Turbopack CSR/1000 modules 2772.44ms ± 7.33ms 2770.35ms ± 5.94ms -0.08%
bench_startup/Turbopack RCC/1000 modules 2601.38ms ± 23.52ms 2654.25ms ± 19.73ms +2.03%
bench_startup/Turbopack RSC/1000 modules 2504.17ms ± 7.44ms 2510.35ms ± 7.72ms +0.25%
bench_startup/Turbopack SSR/1000 modules 2152.07ms ± 7.28ms 2109.64ms ± 5.37ms -1.97% -0.80%

@sokra sokra marked this pull request as ready for review February 7, 2023 22:55
@sokra sokra requested review from a team as code owners February 7, 2023 22:55
Comment on lines +1412 to +1418
let de = &mut serde_json::Deserializer::from_reader(file.read());
match serde_path_to_error::deserialize(de) {
Ok(data) => FileJsonContent::Content(data),
Err(e) => FileJsonContent::Unparseable(
box UnparseableJson::from_serde_path_to_error(e),
),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be using parse_json_rope_with_source_context?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, since we don't want it wrapped in an anyhow error here, but return a structured error.

@mehulkar mehulkar removed their request for review February 7, 2023 23:23
make source context generation more reusable
more visible marker
spacing, marker, make it prettier
@sokra sokra force-pushed the sokra/web-504-serde_json-errors-are-very-bad branch from d4c1fbe to 74b823c Compare February 8, 2023 07:38
@sokra sokra added the pr: automerge Kodiak will merge these automatically after checks pass label Feb 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

Benchmark for 3ea0f81

Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 9385.06µs ± 42.61µs 9837.30µs ± 144.15µs +4.82% +0.83%
bench_hmr_to_eval/Turbopack SSR/1000 modules 8681.57µs ± 107.15µs 8216.92µs ± 71.61µs -5.35% -1.27%
Click to view full benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 9385.06µs ± 42.61µs 9837.30µs ± 144.15µs +4.82% +0.83%
bench_hmr_to_commit/Turbopack RCC/1000 modules 9506.46µs ± 78.69µs 9526.96µs ± 60.50µs +0.22%
bench_hmr_to_commit/Turbopack RSC/1000 modules 481.37ms ± 2.25ms 478.39ms ± 1.12ms -0.62%
bench_hmr_to_commit/Turbopack SSR/1000 modules 10.32ms ± 0.07ms 10.26ms ± 0.08ms -0.63%
bench_hmr_to_eval/Turbopack CSR/1000 modules 8167.21µs ± 73.75µs 8014.63µs ± 90.70µs -1.87%
bench_hmr_to_eval/Turbopack RCC/1000 modules 9432.03µs ± 85.67µs 9132.07µs ± 177.88µs -3.18%
bench_hmr_to_eval/Turbopack SSR/1000 modules 8681.57µs ± 107.15µs 8216.92µs ± 71.61µs -5.35% -1.27%
bench_hydration/Turbopack RCC/1000 modules 4161.44ms ± 8.99ms 4170.03ms ± 13.13ms +0.21%
bench_hydration/Turbopack RSC/1000 modules 3802.25ms ± 14.86ms 3756.24ms ± 13.42ms -1.21%
bench_hydration/Turbopack SSR/1000 modules 3701.29ms ± 14.02ms 3643.89ms ± 22.99ms -1.55%
bench_startup/Turbopack CSR/1000 modules 2750.40ms ± 11.16ms 2730.12ms ± 10.56ms -0.74%
bench_startup/Turbopack RCC/1000 modules 2544.28ms ± 7.65ms 2523.82ms ± 6.13ms -0.80%
bench_startup/Turbopack RSC/1000 modules 2406.36ms ± 8.85ms 2413.93ms ± 7.27ms +0.31%
bench_startup/Turbopack SSR/1000 modules 2086.48ms ± 1.45ms 2083.64ms ± 1.89ms -0.14%

@sokra sokra merged commit 0adae40 into main Feb 8, 2023
@sokra sokra deleted the sokra/web-504-serde_json-errors-are-very-bad branch February 8, 2023 09:17
pub fn write_with_content(&self, writer: &mut impl Write, text: &str) -> std::fmt::Result {
writeln!(writer, "{}", self.message)?;
if let Some(path) = &self.path {
writeln!(writer, " at {}", path)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the at is redundant here. We can remove it and the error would read more easily.

FileJsonContent::Content(data) => {
let js_str_content = serde_json::to_string(data)?;
let inner_code =
format!("__turbopack_export_value__(JSON.parse({js_str_content}));");
Copy link
Member

Choose a reason for hiding this comment

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

This is broken, it no longer "stringifies the string" and just puts the raw JSON into the file

jridgewell pushed a commit to vercel/next.js that referenced this pull request Mar 10, 2023
also make source context generation more reusable

This adds a source context snippet of the JSON that failed parsing and
the json path to the error.

```
An error occurred while generating the chunk item [project]/crates/turbopack-tests/tests/snapshot/imports/json/input/invalid.json (json)
  at Execution of module_factory failed
  at Execution of JsonChunkItem::content failed
  at Unable to make a module from invalid JSON: expected `,` or `}` at line 3 column 26
  at nested.?
     1 | {
     2 |   "nested": {
       |                          v
     3 |     "this-is": "invalid" // lint-staged will remove trailing commas, so here's a comment
       |                          ^
     4 |   }
     5 | }
```
sokra added a commit to vercel/next.js that referenced this pull request Mar 13, 2023
also make source context generation more reusable

This adds a source context snippet of the JSON that failed parsing and
the json path to the error.

```
An error occurred while generating the chunk item [project]/crates/turbopack-tests/tests/snapshot/imports/json/input/invalid.json (json)
  at Execution of module_factory failed
  at Execution of JsonChunkItem::content failed
  at Unable to make a module from invalid JSON: expected `,` or `}` at line 3 column 26
  at nested.?
     1 | {
     2 |   "nested": {
       |                          v
     3 |     "this-is": "invalid" // lint-staged will remove trailing commas, so here's a comment
       |                          ^
     4 |   }
     5 | }
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: automerge Kodiak will merge these automatically after checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants