Skip to content

[Web] Update includes after FFI JSON refactor#18944

Merged
tqchen merged 1 commit intoapache:mainfrom
akaashrp:json
Mar 28, 2026
Merged

[Web] Update includes after FFI JSON refactor#18944
tqchen merged 1 commit intoapache:mainfrom
akaashrp:json

Conversation

@akaashrp
Copy link
Copy Markdown
Contributor

Include ffi/extra/json_parser.cc and ffi/extra/json_writer.cc to maintain compatibility after FFI JSON refactor

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds JSON parsing and writing functionality to the WASM runtime by including the corresponding source files and performs a large-scale update of dependencies in the web package. The review feedback suggests avoiding the direct inclusion of .cc source files to prevent potential One Definition Rule (ODR) violations and recommends separating dependency updates into dedicated pull requests to maintain clarity and reduce the risk of unintended side effects.

Comment thread web/emcc/wasm_runtime.cc
Comment on lines +57 to +58
#include "3rdparty/tvm-ffi/src/ffi/extra/json_parser.cc"
#include "3rdparty/tvm-ffi/src/ffi/extra/json_writer.cc"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Including .cc source files directly is generally discouraged in C++ as it can lead to violations of the One Definition Rule (ODR) and increase build coupling. While this appears to be a pattern in this file, likely for a unity build, it's worth considering if these could be compiled separately and linked, or if the shared code could be moved to header files. If this is a deliberate choice for the WASM build, a comment explaining the rationale would be helpful for future maintainers.

References
  1. Avoid including source files (.cc, .cpp) directly. Instead, include header files (.h, .hpp) and link against the compiled source files. This helps prevent ODR violations and reduces build dependencies.

Comment thread web/package-lock.json
Comment on lines +228 to 236
"version": "7.29.2",
"resolved": "https://registry.npmjs.org/@babel/helpers/-/helpers-7.29.2.tgz",
"integrity": "sha512-HoGuUs4sCZNezVEKdVcwqmZN8GoHirLUcLaYVNBK2J0DadGtdcqgr3BCbvH8+XUo4NGjNl3VOtSjEKNzqfFgKw==",
"dev": true,
"license": "MIT",
"dependencies": {
"@babel/template": "^7.28.6",
"@babel/types": "^7.28.6"
"@babel/types": "^7.29.0"
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

This pull request includes a large number of dependency updates in package-lock.json, but these changes are not mentioned in the PR title or description. It's important to separate dependency updates from feature/bugfix changes into their own PRs for clarity and easier review. If these updates are unintentional, they should be reverted. If they are intentional, please create a separate PR for them with a clear description of why they are needed.

References
  1. Dependency updates should be done in separate, dedicated pull requests. Mixing them with other code changes makes the PR harder to review and increases the risk of introducing unintended side effects.

@tqchen tqchen merged commit 9dcc731 into apache:main Mar 28, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants