Skip to content

Allow package.json "main" to specify a directory#13678

Merged
2 commits merged into
masterfrom
package_json_main_2
Feb 13, 2017
Merged

Allow package.json "main" to specify a directory#13678
2 commits merged into
masterfrom
package_json_main_2

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jan 25, 2017

Fixes #12198
Re-do of #12268

Comment thread src/compiler/moduleNameResolver.ts Outdated
const jsonContent = readJson(packageJsonPath, state.host);
const file = ts ? tryReadFromField("typings") || tryReadFromField("types") : tryReadFromField("main");
if (!file && state.traceEnabled) {
trace(state.host, Diagnostics.package_json_does_not_have_a_0_field, ts ? "types" : "main");
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.

it looks that we'll get this error even in case if package.json has field with some valid name but incorrect type of the value (tryReadFromField in this case will return undefined) - it is a bit misleading

Comment thread src/compiler/moduleNameResolver.ts Outdated
}

function nodeLoadModuleByRelativeName(extensions: Extensions, candidate: string, failedLookupLocations: Push<string>, onlyRecordFailures: boolean, state: ModuleResolutionState): Resolved | undefined {
function nodeLoadModuleByRelativeName(extensions: Extensions, candidate: string, failedLookupLocations: Push<string>, onlyRecordFailures: boolean, state: ModuleResolutionState, considerPackageJson = true): Resolved | undefined {
Copy link
Copy Markdown
Contributor

@vladima vladima Jan 26, 2017

Choose a reason for hiding this comment

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

minor nit: can we convert it to an explicit value? sometimes when reading the code default values for parameters can slip out since they don't appear on the call site

Comment thread src/compiler/moduleNameResolver.ts Outdated
trace(state.host, Diagnostics.Found_package_json_at_0, packageJsonPath);
}

const ts = extensions !== Extensions.JavaScript;
Copy link
Copy Markdown
Contributor

@vladima vladima Jan 26, 2017

Choose a reason for hiding this comment

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

ts is not particularly meaningful, can you give it a better name (i.e. loadWithTsExtensions)?

// Even if extensions is DtsOnly, we can still look up a .ts file as a result of package.json "types"
const nextExtensions = extensions === Extensions.DtsOnly ? Extensions.TypeScript : extensions;
// Don't do package.json lookup recursively, because Node.js' package lookup doesn't.
return nodeLoadModuleByRelativeName(nextExtensions, file, failedLookupLocations, onlyRecordFailures, state, /*considerPackageJson*/ false);
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.

that looks a bit odd: does it mean that i.e. for .ts or .js extensions we'll repeat the whole resolution process trying to resolve the same file again?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We'll repeat the process trying to resolve the file specified in the package.json. But considerPackageJson is false this time, so it won't repeat forever.

@ghost ghost merged commit d24b689 into master Feb 13, 2017
@ghost ghost deleted the package_json_main_2 branch February 13, 2017 14:19
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants