Skip to content

esm: Add support for pjson cache and main without extensions #18728

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

Closed
guybedford wants to merge 13 commits into from
Closed

esm: Add support for pjson cache and main without extensions #18728

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f14420e
esm: support main without extension and pjson cache
guybedford Feb 12, 2018
adfe1ac
pr feedback
guybedford Feb 12, 2018
ebdb3cb
explicit isMain checks
guybedford Feb 12, 2018
5855e5c
loader setup promise tweaks
guybedford Feb 12, 2018
05c9941
further tweaks
guybedford Feb 12, 2018
2d6d4e6
c++ feedback
guybedford Feb 12, 2018
a06db2d
better todo for JSON parse failure
guybedford Feb 12, 2018
081aab4
invalid json handling todo
guybedford Feb 12, 2018
ed9df8a
naming adjustments, simplifications
guybedford Feb 12, 2018
8b4d85f
move PackageConfig into loader namespace
guybedford Feb 12, 2018
4468e2c
make PackageConfig consts
guybedford Feb 12, 2018
b4dc802
clean up emplace logic a little
guybedford Feb 13, 2018
9f7aabd
boolean aliasing
guybedford Feb 14, 2018
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
Prev Previous commit
Next Next commit
invalid json handling todo
  • Loading branch information
@guybedford
guybedford committed Feb 12, 2018
commit 081aab4ab394cc06012edd1b5a66454682be1a5e
1 change: 1 addition & 0 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,7 @@ class Environment {

struct PackageConfig {
bool exists;
bool is_valid;
bool has_main;
std::string main;
};
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Feb 12, 2018

Choose a reason for hiding this comment

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

PackageConfig should probably live in the node::loader namespace, like ModuleWrap.

Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Feb 12, 2018

Choose a reason for hiding this comment

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

Would still be nice if these were const.

Copy link
Copy Markdown
Contributor Author

@guybedford guybedford Feb 12, 2018

Choose a reason for hiding this comment

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

As far as I can tell getting this to work would mean inlining the kEmptyPackage constructor at each place it is assigned through an emplace call, which doesn't seem very nice.

Copy link
Copy Markdown
Contributor Author

@guybedford guybedford Feb 12, 2018

Choose a reason for hiding this comment

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

(as in kEmptyPackage would be replaced by these inline constructors?)

Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Feb 12, 2018

Choose a reason for hiding this comment

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

map.emplace(std::make_pair(key, kEmptyPackage)) should work, I think. If it doesn't, you could add a PackageConfig() = default constructor and emplace PackageConfig() instead.

Copy link
Copy Markdown
Contributor Author

@guybedford guybedford Feb 12, 2018

Choose a reason for hiding this comment

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

The first case didn't work unfortunately. In the second case it would need to be a PackageConfig(true) and a PackageConfig(false) to avoid a full expansion of arguments. But = default doesn't seem to be compatible with default constructor arguments as far as I can tell?

Copy link
Copy Markdown
Contributor Author

@guybedford guybedford Feb 12, 2018

Choose a reason for hiding this comment

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

I managed to get the consts to work out, but it's pretty awful code to look at.

Expand Down
15 changes: 9 additions & 6 deletions src/module_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,10 @@ Maybe<uv_file> CheckFile(const std::string& path,

const Environment::PackageConfig& GetPackageConfig(Environment* env,
const std::string path) {
static const Environment::PackageConfig kEmptyPackage = { false, false, "" };
static const Environment::PackageConfig kEmptyPackage =
{ false, true, false, "" };
static const Environment::PackageConfig kInvalidPackage =
{ true, false, false, "" };

auto existing = env->package_json_cache.find(path);
if (existing != env->package_json_cache.end())
Expand Down Expand Up @@ -528,11 +531,9 @@ const Environment::PackageConfig& GetPackageConfig(Environment* env,
Local<Value> pkg_json_v;
Local<Object> pkg_json;

// TODO: Invalid package.json MUST THROW the resolve
// currently this will just silently ignore
if (!JSON::Parse(env->context(), src).ToLocal(&pkg_json_v) ||
!pkg_json_v->ToObject(env->context()).ToLocal(&pkg_json))
return kEmptyPackage;
return env->package_json_cache[path] = kInvalidPackage;

Local<Value> pkg_main;
bool has_main = false;
Expand All @@ -544,7 +545,7 @@ const Environment::PackageConfig& GetPackageConfig(Environment* env,
main_std = std::string(*main_utf8, main_utf8.length());
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Feb 12, 2018

Choose a reason for hiding this comment

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

Maybe use main_std.assign() here. The compiler will probably elide the temporary but .assign() will for sure.

}

Environment::PackageConfig pjson = { true, has_main, main_std };
Environment::PackageConfig pjson = { true, true, has_main, main_std };
return env->package_json_cache[path] = pjson;
}
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Feb 12, 2018

Choose a reason for hiding this comment

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

Return either a const PackageConfig& or a PackageConfig*. Mutable references are tricky because it's not always clear at the call site if you're manipulating the original or a copy.

Copy link
Copy Markdown
Member

@benjamingr benjamingr Feb 13, 2018

Choose a reason for hiding this comment

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

It's not shown here, but this issue is addressed and const PackageConfig& is returned.


Expand Down Expand Up @@ -583,7 +584,9 @@ Maybe<URL> ResolveMain(Environment* env, const URL& search) {

const Environment::PackageConfig& pjson =
GetPackageConfig(env, pkg.ToFilePath());
if (!pjson.exists || !pjson.has_main) {
// Note invalid package.json should throw in resolver
// currently we silently ignore which is incorrect
if (!pjson.exists || !pjson.is_valid || !pjson.has_main) {
return Nothing<URL>();
}
if (!ShouldBeTreatedAsRelativeOrAbsolutePath(pjson.main)) {
Expand Down