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

Support loading .mjs files with async API #46

Merged
merged 17 commits into from
Feb 14, 2024
Merged

Support loading .mjs files with async API #46

merged 17 commits into from
Feb 14, 2024

Conversation

antonk52
Copy link
Owner

@antonk52 antonk52 commented Feb 4, 2024

This PR supersedes #45

We should be able to be able to load native esm modules with lilconfig. To do this a few changes have to be made

  • Use native dynamic imports to load js/mjs/cjs when using async API
  • Make sure that sync API still uses require by separating defaultLoaders and defaultLoadersSync
  • Make sure only async API has .mjs in search places

The following is notes from while I was researching and implementing this

  • update nodejs req to node v14 mdn
  • mention in readme version compatibility added mjs loading in cosmiconfig 8.2.0 see changelog, async api only
  • typescript builds dynamic import into Promise.resolve().then(() => require(x)), workaround
  • node/jest option to support native dynamic import, update test npm script
  • changed message format for missing loader for extension
    Aside: skeptical if we should make this change. Previous error message is subjectively clearer. Might revert the commit with these changes.
  • cosmiconfig stopped throwing for empty string passed to searcher.load()
    Aside: not sure if this should be ported either. New error message from node itself is not helpful and will be hard to debug for end users
  • separate default loaders based on sync/async version
  • cosmiconfig diverged a lot to the point that it might not be worth it to remain compatible
  • test with node < 18.x.x
  • consider supporting meta search places
  • update compatibility table with cosmiconfig as the functionality has diverged

@antonk52
Copy link
Owner Author

antonk52 commented Feb 5, 2024

I noticed that tests fail on node v14/16/18 with a segmentation fault. Things I tried

  • updating jest from 27 to 28 and 29. Same segfaults on older node versions
  • migrating to vitest which, required some configuration, specifically launched as NODE_OPTIONS=--experimental-vm-modules ./node_modules/.bin/vitest --pool, (--pool for vm modules). This worked for the most part, but 5 tests were failing with a stack trace leading to import-fresh which cosmiconfig uses to import js modules to avoid commonjs cache.
  • native nodejs test is not an option as it is not available in older nodejs versions

I see a few of paths forward:

  • Stop testing node.js versions older than 20, which is pretty unfortunate
  • Try out uvu which might be runnable on older nodejs versions. Need more time to test it out, specifically around mocking.
  • Add another test file. It won't use a testing framework. It will plainly use native assert to verify that it works. The module will load some of the configs similar to ones from jest test cases. This will ensure basic backward compatibility with older nodejs versions.

UPD: I falsly credited only import-fresh for the segfault. I can repro the segfault with eval('import(${id})'). The odd part is that if I inline eval('import("/absolute/file/path/to/file.mjs")') in the test, it works as expected. But when it is in external file it segfaults. Primarily with --coverage too. I tried creating a js file in the repo root with a plain js with an example below and it worked. Which makes me think that the issue is still jest specific.

// node script.js
const {lilconfig} = require('./dist/index.js');
const path = require('path');

async function main() {
    const stopDir = __dirname;
    const searchFrom = path.join(stopDir, 'src', 'spec', 'esm-project', 'foo', 'bar', 'baz');
    const result = await lilconfig('esm', {stopDir}).search(searchFrom);
    console.log(result); // {esm: true}
}
main();

At this point I am fine with keeping the main test suit, linter and build to run on the last node.js only and extracting a few main use cases to be tested with uvu on a matrix of older 14/16/18 nodejs versions.

@antonk52
Copy link
Owner Author

I think this PR is finally ready to land. If all goes well I will be releasing the new version this week

@antonk52
Copy link
Owner Author

Since I am not supporting any of the features that were added to cosmiconfig in v9, I won't be updating readme regarding these features. Finally merging

@antonk52 antonk52 merged commit 6feff0d into master Feb 14, 2024
4 of 6 checks passed
@antonk52 antonk52 deleted the feat/esm branch February 14, 2024 21:04
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.

2 participants