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.

Sign up for GitHub

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

src: rewrite BindingData::GetPackageScopeConfig, don't use url parser #54316

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

src: rewrite BindingData::GetPackageScopeConfig, don't use url parser #54316

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8de64c
src: rewrite BindingData::GetPackageScopeConfig, don't use url parser
BalaM314 Aug 11, 2024
c23eb84
Apply suggestions from code review
BalaM314 Aug 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 27 additions & 31 deletions src/node_modules.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,60 +378,56 @@ void BindingData::GetPackageScopeConfig(

Realm* realm = Realm::GetCurrent(args);
Utf8Value resolved(realm->isolate(), args[0]);
auto package_json_url_base = ada::parse(resolved.ToStringView());
if (!package_json_url_base) {

auto current_path_url = ada::parse(resolved.ToStringView());
BalaM314 marked this conversation as resolved.
Show resolved Hide resolved
if (!current_path_url) {
url::ThrowInvalidURL(realm->env(), resolved.ToStringView(), std::nullopt);
return;
}
auto package_json_url =
ada::parse("./package.json", &package_json_url_base.value());
if (!package_json_url) {
url::ThrowInvalidURL(realm->env(), "./package.json", resolved.ToString());
return;
}

std::string_view node_modules_package_path = "/node_modules/package.json";
auto current_path_opt = url::FileURLToPath(realm->env(), *current_path_url);
CHECK(current_path_opt);
// BufferValue path_buffer(realm->isolate(), current_path_opt.value());
// // Check if the path has a trailing slash. If so, add it after
// // ToNamespacedPath() as it will be deleted by ToNamespacedPath()
// bool endsWithSlash = path_buffer.ToStringView().ends_with(
// std::filesystem::path::preferred_separator);
// ToNamespacedPath(realm->env(), &path_buffer);
// std::string path_value_str = path_buffer.ToString();
// if (endsWithSlash) {
// path_value_str.push_back(std::filesystem::path::preferred_separator);
// }
Comment on lines +390 to +399
Copy link
Member

Choose a reason for hiding this comment

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

I didn't understand the purpose of this comments. Did you forget them?

Suggested change
// BufferValue path_buffer(realm->isolate(), current_path_opt.value());
// // Check if the path has a trailing slash. If so, add it after
// // ToNamespacedPath() as it will be deleted by ToNamespacedPath()
// bool endsWithSlash = path_buffer.ToStringView().ends_with(
// std::filesystem::path::preferred_separator);
// ToNamespacedPath(realm->env(), &path_buffer);
// std::string path_value_str = path_buffer.ToString();
// if (endsWithSlash) {
// path_value_str.push_back(std::filesystem::path::preferred_separator);
// }

Copy link
Author

Choose a reason for hiding this comment

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

ToNamespacedPath needs to be called here, as current_path_opt is the non-UNC path and won't work if it's longer than 260 characters, but that function requires a BufferValue which requires a v8 value
I was planning to change ToNamespacedPath to accept a string, as it suggests here

auto current_path = std::filesystem::path(current_path_opt.value());

auto error_context = ErrorContext();
error_context.is_esm = true;

// TODO(@anonrig): Rewrite this function and avoid calling URL parser.
while (true) {
auto pathname = package_json_url->get_pathname();
if (pathname.ends_with(node_modules_package_path)) {
if (current_path.generic_string().ends_with("/node_modules")) {
break;
}

auto file_url = url::FileURLToPath(realm->env(), *package_json_url);
CHECK(file_url);
error_context.specifier = resolved.ToString();
auto package_json = GetPackageJSON(realm, *file_url, &error_context);
auto package_json = GetPackageJSON(realm, (current_path / "package.json").string(), &error_context);
BalaM314 marked this conversation as resolved.
Show resolved Hide resolved
if (package_json != nullptr) {
return args.GetReturnValue().Set(package_json->Serialize(realm));
}

auto last_href = std::string(package_json_url->get_href());
auto last_pathname = std::string(package_json_url->get_pathname());
package_json_url = ada::parse("../package.json", &package_json_url.value());
if (!package_json_url) {
url::ThrowInvalidURL(realm->env(), "../package.json", last_href);
return;
}

// Terminates at root where ../package.json equals ../../package.json
// (can't just check "/package.json" for Windows support).
if (package_json_url->get_pathname() == last_pathname) {
auto parent_path = current_path.parent_path();
//If the parent directory is the same as the current directory, stop searching
BalaM314 marked this conversation as resolved.
Show resolved Hide resolved
if (parent_path == current_path) {
break;
}
current_path = parent_path;
}

auto package_json_url_as_path =
url::FileURLToPath(realm->env(), *package_json_url);
CHECK(package_json_url_as_path);
//No package.json found, return the last searched path
BalaM314 marked this conversation as resolved.
Show resolved Hide resolved
auto package_json_path = (current_path / "package.json").string();
BalaM314 marked this conversation as resolved.
Show resolved Hide resolved
return args.GetReturnValue().Set(
String::NewFromUtf8(realm->isolate(),
package_json_url_as_path->c_str(),
package_json_path.c_str(),
NewStringType::kNormal,
package_json_url_as_path->size())
package_json_path.size())
.ToLocalChecked());
}

Expand Down
Loading