-
Notifications
You must be signed in to change notification settings - Fork 598
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
[WIP] Draft PR for adding imports
to package.json
#995
base: main
Are you sure you want to change the base?
[WIP] Draft PR for adding imports
to package.json
#995
Conversation
Implement `resolvePackageTargetsFromImports`as per spec at : https://nodejs.org/api/esm.html#resolver-algorithm-specification
import path from 'path'; | ||
import fs from 'fs'; | ||
import * as url from 'url'; | ||
export function resolvePackageTargetsFromImports(specifier: string, parentURL, conditions){ |
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.
Please update this function to use the existing ResolutionContext
object, which provides APIs such as context.getPackageForModule()
and context.doesFileExist()
.
- This should enable your implementation to work without importing and using
fs
. - Removes the need for separate
lookupParentScope
andreadPackageJSON
utils. - Enables this function to read config options such as
resolver.unstable_conditionNames
.
The function should also be named to reflect returning a singular Target
(typo). Together, the signature might look closer to:
export function resolvePackageTargetFromImports(
context: ResolutionContext,
importSpecifier: string,
importsField: ExportMap,
platform: string | null,
): FileResolution {
This also means we should move the specifier.startsWith('#')
check externally, to around resolve.js#L52 and call this function with the parsed imports
field if it exists.
} | ||
|
||
if(specifier.startsWith('#/') || specifier === '#') { | ||
throw Error('Invalid Module Specifier.'); |
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 should be replaced with a dedicated error type — this existed previously and can be copied over: https://github.com/facebook/metro/blob/47856447a885d695f781694e5200ca2c3adecc87/packages/metro-resolver/src/errors/InvalidModuleSpecifierError.js
Similarly for any other errors we're introducing from the Node.js spec.
return null; | ||
} | ||
|
||
function packageTargetResolve(packageURL, target, patternMatch, isImports, conditions) { |
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.
Do we need to Implement the methods
PACKAGE_IMPORTS_RESOLVE
depends on[?]
We don't! 😄 packageTargetResolve
and PackageExportsResolve
are already implemented elsewhere in metro-resolver
— please remove.
While this lines up 1:1 with the Node.js resolution algorithm, let's reuse what's available in the rest of the resolver code rather than reimplementing here — which (while not an exact match with Node's strategy) will respect other Metro resolution behaviours such as resolver.extraNodeModules
.
Please look to locate the function call for resolvePackageTargetsFromImports()
within the main resolve()
function and use resolve()
to evaluate the package target read from "imports"
(updating context.originModulePath
when calling this).
The control flow would look roughly like:
- Within
resolve()
- if the import specifier (
moduleName
) begins with#
AND ifimports
is incontext.getPackageForModule(context.originModulePath)
- get the result of
resolvePackageTargetFromImports(context, moduleName, importsField, ...)
- if a
target
was returned (containing the matched import specifier value from"imports"
), call and returnresolve(target)
- else, fall through (will error after all other resolution steps)
- get the result of
- if the import specifier (
@huntie : Thanks for taking the time to do a detailed PR review! I'll address the feedback 🙏🏻 |
Resuming work on this. |
@siddarthkay do you have plans to continuing working on this? Alternatively, I'd be interested in taking over the assignment of the original issue. |
HI @kraenhansen : hmm, I'd be happy if you took it over. Thank you. |
cc @huntie |
This is a WIP PR to just collect my thoughts and to ask questions about this implementation.
fixes #978
Summary
PACKAGE_IMPORTS_RESOLVE
insidepackages/metro-resolver/src/PackageImportsResolve.js
as per spec at https://nodejs.org/api/esm.html#resolver-algorithm-specificationPACKAGE_IMPORTS_RESOLVE
depends on like ?:packages/metro-resolver/src/PackageImportsResolve.js
Test plan
tests must pass by running
npm test