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

added submodule tests #26

Merged
merged 25 commits into from
Aug 23, 2022
Merged

added submodule tests #26

merged 25 commits into from
Aug 23, 2022

Conversation

iambumblehead
Copy link
Owner

adds tests and changes for subpath exports, tests the export patterns from the documentation site https://nodejs.org/api/packages.html#package-entry-points

{
  "exports": {
    ".": "./index.js",
    "./submodule.js": "./src/submodule.js"
  }
}

@iambumblehead
Copy link
Owner Author

this will not be ready tonight/today

@iambumblehead
Copy link
Owner Author

parts of PACKAGE_RESOLVE(packageSpecifier, parentURL) from https://nodejs.org/api/esm.html need to be followed

scoped packages would be affected so scoped package tests should be added to the main test file

@iambumblehead
Copy link
Owner Author

iambumblehead commented Aug 13, 2022

tests are passing, but a few extra things are needed,

  • the subpath lookup needs to be updated to handle subpath globby patterns,
  • one esm export lookup should be shared/reused by for subpath lookups and package man/index lookups
  • probably, the export lookup should recursively process export lists and object definitions
  • tests and support should be added for "#subpath"

@iambumblehead
Copy link
Owner Author

the "index.js" files should be renamed eg "index.test.js" so they won't be returned through legacy cjs-resolving functions where "index.js" is automatically returned.

@iambumblehead iambumblehead force-pushed the support-subpath-exports branch 2 times, most recently from c631758 to e9118ac Compare August 17, 2022 14:29
@@ -191,6 +203,91 @@ export default (o => {
return o.getasfilesync(temppath, opts) || o.getasdirsync(temppath, opts);
};

o.getasesmimportpathfrompjson = (targetpath, specifier, pjson) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nits: the func name is really hard to read. lol.

Copy link
Owner Author

Choose a reason for hiding this comment

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

haha ok

Copy link
Owner Author

Choose a reason for hiding this comment

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

this was renamed to "esmparseimportpkg" if you prefer a different name it could be changed and readability improvements could be made in a follow up PR

@iambumblehead
Copy link
Owner Author

note to myself mostly. resolvewithplus should be updated to return file urls like 'file:///path/to/script.js' and 'node:fs' to match the import.meta.resolve behaviour and to save esmock from the need to check if protocol is defined or to add the protocol

@iambumblehead
Copy link
Owner Author

Everything is working here and the tests are passing.

A few refinements that could be made but haven't been made yet,

  • reuse one parsing function for both import and export patterns,
  • remove export pattern parsing from the cjs package.json filepaths resolving function,
  • improve function names and variable names or readability

I don't know when these improvements will be made. This is a dull task and my energy is low atm.

@iambumblehead
Copy link
Owner Author

tests for these nested and conditional export definitions should be added as well :/

https://nodejs.org/api/packages.html#conditional-exports

{
  "exports": {
    ".": "./index.js",
    "./feature.js": {
      "node": "./feature-node.js",
      "default": "./feature.js"
    }
  }
}

https://nodejs.org/api/packages.html#conditional-exports

{
  "exports": {
    "node": {
      "import": "./feature-node.mjs",
      "require": "./feature-node.cjs"
    },
    "default": "./feature.mjs"
  }
}

@iambumblehead
Copy link
Owner Author

one function is now used to parse spec patterns to obtain,

  1. package root index,
  2. getting a package's exported specifier and
  3. getting a package's imported specifier

it will be ready for review after tests for the patterns above are added and function names are improved.

@iambumblehead
Copy link
Owner Author

@tripodsan @aladdin-add this PR may be too annoying to review. It has become a huge PR. If you will review this that's great and if not it is understood why.

Some function names are changed a little bit, but imo it would be easier to refine things more in a follow up PR. There is something off about linting and/or my editor that needs to be looked into and probably we should remove the "o.functioname" pattern and use something like "const functionName" or whatever.

This PR completes the objective of resolving esm patterns from node.js' documentation examples

@iambumblehead iambumblehead changed the title added submodule tests (failing) added submodule tests Aug 21, 2022
@iambumblehead
Copy link
Owner Author

merging this!

@iambumblehead iambumblehead merged commit a892306 into master Aug 23, 2022
@iambumblehead iambumblehead deleted the support-subpath-exports branch August 23, 2022 03:34
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