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

resolve named-properties that are also asterisk folder name #65

Merged
merged 9 commits into from
Jan 23, 2024

Conversation

iambumblehead
Copy link
Owner

@iambumblehead iambumblehead commented Jan 23, 2024

cc @koshic (also @dschnare)

This PR is for resolving iambumblehead/esmock#289 eg, importing a moduleId "mymodule/mystuff" should return "mymodule/src/mystuff/index.js" given the below "mymodule" package.json,

{
  "name": "mymodule",
  "type": "module",
  "exports": {
    "./*": {
      "default": "./src/*/index.js",
      "types": "./types/*/index.d.ts"
    }
  }
}

While changes here may not be great, they only occur inside specific conditions that would currently fail every time. A few function calls were updated to include the "opts" param, beecause "opts" are needed by the esmparse function which is called in new place here.

First, the script checks to see if the property name includes an asterisk and if the definition is an object

if (isobj(esmval) && esmkey.includes('*')) {
  // ...
}

If that condition is true it generates an "expanded" version of the spec. eg, for moduleId "stuff",

{
  default: './src/mystuff/index.js',
  types: './types/mystuff/index.d.ts'
}

and tries to resolve the moduleId using the "expanded" spec value

esmparse({
  default: './src/mystuff/index.js',
  types: './types/mystuff/index.d.ts'
}, 'mystuff', opts) // ./src/mystuff/index.js

@iambumblehead
Copy link
Owner Author

on second look, there might be better ways I can think of for doing this one... maybe tomorrow I can try something else

@iambumblehead iambumblehead removed the request for review from koshic January 23, 2024 08:42
exp[nestkey] = getesmkeyvalglobreplaced(
expandedkey,
path.posix.join(pathfirstdir, expandedkey),
path.posix.join(resolvedkey, esmval[nestkey].split('*')[1]))

Choose a reason for hiding this comment

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

I'm not sure this will support multiple occurrences of * on the "right side".

All instances of * on the right hand side will then be replaced with this value, including if it contains any / separators.

// import { stuff } from 'my-module/stuff' <-- should resolve to "my-module/src/stuff/some-folder/stuff/index.js"
{
  "name": "my-module"
 "exports": {
    "./*": {
      "default": "./src/*/some-folder/*/index.js"
    }
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

There are multiple situations that aren't handled here now and if there's no objection, maybe this area could be improved with follow up PRs that each have their own unit-test.

I google searched multiple wildcard exports last night and found reports of differences and some breakage between typescript and different runtimes. This seems like a feature that is not commonly used.

Copy link
Owner Author

Choose a reason for hiding this comment

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

eg, this would be another situation that wouldn't be expanded correctly here now (which is probably "okay")

{
  "name": "my-module"
  "exports": {
    "./feature*": {
      "default": "./src/features-*/index.js"
    }
}

@iambumblehead
Copy link
Owner Author

going to merge and publish this now. if any changes or suggestions or needed they can be applied with follow up MRs

@iambumblehead iambumblehead merged commit d3e2912 into main Jan 23, 2024
5 checks passed
@iambumblehead iambumblehead deleted the resolve-asterisk-folder-name branch January 23, 2024 19:22
@dschnare
Copy link

@iambumblehead Thanks for resolving this and moving so quickly. I appreciate the time you spent on this.

@iambumblehead
Copy link
Owner Author

@dschnare thank you I'm very happy if I can be a useful person :)

@iambumblehead
Copy link
Owner Author

@dschnare it would interesting if the asterisk-matching/expanding behaviour were updated to work in a robust way...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants