-
Notifications
You must be signed in to change notification settings - Fork 123
feat: use serde to build package json and check npm package name #2428
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,7 +8,7 @@ use std::{ | |||||||||||||||||
| }; | ||||||||||||||||||
| use stellar_xdr::curr::ScSpecEntry; | ||||||||||||||||||
|
|
||||||||||||||||||
| use super::generate; | ||||||||||||||||||
| use super::{generate, validate_npm_package_name}; | ||||||||||||||||||
|
|
||||||||||||||||||
| static PROJECT_DIR: Dir<'_> = include_dir!("$CARGO_MANIFEST_DIR/src/project_template"); | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -52,6 +52,14 @@ impl Project { | |||||||||||||||||
| network_passphrase: Option<&str>, | ||||||||||||||||||
| spec: &[ScSpecEntry], | ||||||||||||||||||
| ) -> std::io::Result<()> { | ||||||||||||||||||
| validate_npm_package_name(contract_name).map_err(|e| { | ||||||||||||||||||
| std::io::Error::new( | ||||||||||||||||||
| std::io::ErrorKind::InvalidInput, | ||||||||||||||||||
| format!( | ||||||||||||||||||
| "output directory name '{contract_name}' is not a valid npm package name: {e}" | ||||||||||||||||||
| ), | ||||||||||||||||||
| ) | ||||||||||||||||||
| })?; | ||||||||||||||||||
| self.replace_placeholder_patterns(contract_name, contract_id, rpc_url, network_passphrase)?; | ||||||||||||||||||
| self.append_index_ts(spec, contract_id, network_passphrase) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
@@ -87,7 +95,12 @@ impl Project { | |||||||||||||||||
| ), | ||||||||||||||||||
| ]; | ||||||||||||||||||
| let root: &Path = self.as_ref(); | ||||||||||||||||||
| ["package.json", "README.md", "src/index.ts"] | ||||||||||||||||||
|
|
||||||||||||||||||
| // Handle package.json with proper JSON serialization | ||||||||||||||||||
| self.replace_package_json(root, contract_name)?; | ||||||||||||||||||
|
|
||||||||||||||||||
| // Handle non-JSON files with string replacement | ||||||||||||||||||
| ["README.md", "src/index.ts"] | ||||||||||||||||||
| .into_iter() | ||||||||||||||||||
| .try_for_each(|file_name| { | ||||||||||||||||||
| let file = &root.join(file_name); | ||||||||||||||||||
|
|
@@ -99,6 +112,33 @@ impl Project { | |||||||||||||||||
| }) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| fn replace_package_json(&self, root: &Path, contract_name: &str) -> std::io::Result<()> { | ||||||||||||||||||
| let file = root.join("package.json"); | ||||||||||||||||||
| let contents = fs::read_to_string(&file)?; | ||||||||||||||||||
| let mut json: serde_json::Value = serde_json::from_str(&contents).map_err(|e| { | ||||||||||||||||||
| std::io::Error::new( | ||||||||||||||||||
| std::io::ErrorKind::InvalidData, | ||||||||||||||||||
| format!("failed to parse package.json template: {e}"), | ||||||||||||||||||
| ) | ||||||||||||||||||
| })?; | ||||||||||||||||||
|
|
||||||||||||||||||
| if let Some(obj) = json.as_object_mut() { | ||||||||||||||||||
| obj.insert( | ||||||||||||||||||
| "name".to_string(), | ||||||||||||||||||
| serde_json::Value::String(contract_name.to_string()), | ||||||||||||||||||
| ); | ||||||||||||||||||
|
||||||||||||||||||
| ); | |
| ); | |
| } else { | |
| return Err(std::io::Error::new( | |
| std::io::ErrorKind::InvalidData, | |
| "package.json template must be a JSON object", | |
| )); |
Copilot
AI
Mar 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test hard-codes the complete set of top-level keys in package.json and will fail if the template is legitimately extended (e.g., adding license, files, keywords). To reduce brittleness, consider asserting that the expected keys are present (and/or that key count is unchanged) rather than rejecting any new keys.
| for key in obj.keys() { | |
| assert!( | |
| expected_keys.contains(&key.as_str()), | |
| "unexpected key in package.json: {key}" | |
| for key in expected_keys { | |
| assert!( | |
| obj.contains_key(key), | |
| "missing expected key in package.json: {key}" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,8 @@ pub enum Error { | |
| Spec(#[from] spec_tools::Error), | ||
| #[error("Failed to get file name from path: {0:?}")] | ||
| FailedToGetFileName(PathBuf), | ||
| #[error("--output-dir basename is not a valid npm package name: {0}. Use only lowercase alphanumeric characters, hyphens, dots, and underscores")] | ||
| InvalidContractName(String), | ||
| #[error(transparent)] | ||
| WasmOrContract(#[from] contract_spec::Error), | ||
| #[error(transparent)] | ||
|
|
@@ -82,6 +84,8 @@ impl Cmd { | |
| let contract_name = &file_name | ||
| .to_str() | ||
| .ok_or_else(|| Error::NotUtf8(file_name.to_os_string()))?; | ||
| soroban_spec_typescript::validate_npm_package_name(contract_name) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. validate_npm_package_name is called here and then again in |
||
| .map_err(Error::InvalidContractName)?; | ||
|
Comment on lines
+87
to
+88
|
||
| let (resolved_address, network) = match source { | ||
| contract_spec::Source::Contract { | ||
| resolved_address, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't use
self, so maybe we could extract it as a static function?