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

fix: Resolve config modules with ESM createRequire() #1082

Merged
merged 4 commits into from Jan 7, 2022

Conversation

evocateur
Copy link
Contributor

ES modules do not have require() available, we must construct it first.
This fixes the CLI when passing --config my-config-package, broken in v12.0.0.

The tests didn't catch this because Jest still doesn't support mocking ESM, and there's really no way to write a test for this right now. I verified it locally like this:

Broken

// broken.mjs
console.log(require.resolve('react'))
$ node -v
v14.18.1

$ node ./broken.mjs
file:///path/to/broken.mjs:1
console.log(require.resolve('react'))
            ^

ReferenceError: require is not defined in ES module scope, you can use import instead
    at file:///Users/daniel/code/front-end-shared-config/resolve-config.mjs:1:13
    at ModuleJob.run (internal/modules/esm/module_job.js:183:25)
    at async Loader.import (internal/modules/esm/loader.js:178:24)
    at async Object.loadESM (internal/process/esm_loader.js:68:5)

Fixed

// fixed.mjs
import { createRequire } from 'module'

const require = createRequire(import.meta.url)

console.log(require.resolve('react'))
$ node ./fixed.mjs
/path/to/node_modules/react/index.js

@iiroj
Copy link
Member

iiroj commented Jan 6, 2022

Thanks for noticing! The tests don't seem to pass, unfortunately.

@iiroj
Copy link
Member

iiroj commented Jan 6, 2022

@evocateur can you move resolveConfig into its own file, and mock it to use require.resolve during tests as a workaround?

I've been meaning to rewrite the tests in ESM as well but it's a huge pain with all the mocking we have.

@evocateur
Copy link
Contributor Author

@iiroj Ah, dang, that makes sense. Thought I could get away with not running the tests locally. 😢 I'll fix it momentarily.

ES modules do not have require() available, we must construct it first.
This fixes the CLI when passing `--config my-config-package`, broken in
v12.0.0.

The tests didn't catch this because Jest still doesn't support mocking
ESM, and there's really no way to write a test for this right now.

I wasn't able to get the automatic `test/__mocks__/resolveConfig.js` to
work, probably a race condition between the module parser and the mocker.
@evocateur
Copy link
Contributor Author

@iiroj I feel your pain with the Jest ESM mocking story.

I wasn't able to get the automatic test/__mocks__/resolveConfig.js pattern to work (probably a race condition between the module parser and the mocker), so I had to copy and paste that wee chunk four places. Then I hit local integration test failures, related to my git config --global init.defaultBranch setting of main, which I fixed in a separate commit.

Thanks for your responsiveness!

lib/loadConfig.js Outdated Show resolved Hide resolved
@iiroj iiroj self-requested a review January 7, 2022 16:00
@codecov
Copy link

codecov bot commented Jan 7, 2022

Codecov Report

Merging #1082 (1321674) into master (893f3d7) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1082   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines          593       589    -4     
  Branches       159       159           
=========================================
- Hits           593       589    -4     
Impacted Files Coverage Δ
lib/loadConfig.js 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 893f3d7...1321674. Read the comment docs.

@iiroj iiroj merged commit f9f6538 into lint-staged:master Jan 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2022

🎉 This PR is included in version 12.1.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

2 participants