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

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

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

Conversation

BalaM314
Copy link

Will fix #54304 when completed
Rewrote BindingData::GetPackageScopeConfig to use std::filesystem::path instead of URL parser
TODO:

  • modify ToNamespacedPath to accept a std::string as argument
  • modify ToNamespacedPath to optionally keep the trailing slash
  • call ToNamespacedPath in BindingData::GetPackageScopeConfig
  • add tests

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 11, 2024
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Thank you for doing this. Some small changes are required but in overall, I think this is the correct path forward

src/node_modules.cc Outdated Show resolved Hide resolved
src/node_modules.cc Outdated Show resolved Hide resolved
src/node_modules.cc Outdated Show resolved Hide resolved
src/node_modules.cc Outdated Show resolved Hide resolved
src/node_modules.cc Outdated Show resolved Hide resolved
Comment on lines +390 to +399
// 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
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

Authored-By: Yagiz Nizipli <[email protected]>

Co-authored-by: Yagiz Nizipli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node does not determine that a file is an ES module when the package.json is in a long path
3 participants