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

Add support for vocab.config.cjs #176

Merged
merged 4 commits into from
Nov 12, 2023
Merged

Conversation

rstacruz
Copy link
Collaborator

@rstacruz rstacruz commented Nov 9, 2023

Context

Vocab currently supports vocab.config.js.

However, when using Vocab in ESM projects, Vocab may throw an error like this:

Error [ERR_REQUIRE_ESM]: require() of ES Module /[project]/vocab.config.js from /[project]/node_modules/@vocab/core/dist/vocab-core.cjs.dev.js not supported.

[1] vocab.config.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains “type”: “module” which declares all .js files in that package scope as ES modules.

Vocab tries to do require('./path/to/vocab.config.js'), which shouldn't be possible inside an ESM workspace. vocab.config.js is assumed to be an ESM module, when it's actually a CJS module.

Solution

This adds support for vocab.config.cjs.

Caveats

Unfortunately, support for vocab.config.mjs won't be straight-forward.

A workaround is available as well: vocab compile --config vocab.config.cjs

Copy link

changeset-bot bot commented Nov 9, 2023

🦋 Changeset detected

Latest commit: 61880a8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@vocab/core Minor
@vocab/cli Patch
@vocab/phrase Patch
@vocab/react Patch
@vocab/types Patch
@vocab/webpack Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@rstacruz rstacruz marked this pull request as ready for review November 9, 2023 23:50
@rstacruz rstacruz requested a review from a team as a code owner November 9, 2023 23:50
Copy link
Contributor

@mrm007 mrm007 left a comment

Choose a reason for hiding this comment

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

LGTM, but would appreciate a fixture or a unit test with a .cjs config.

There's also undefined unspecified behaviour (to consumers) when both a vocab.config.js and vocab.config.cjs are present. We could do better, but I'm not against merging this PR as-is.

@askoufis
Copy link
Contributor

LGTM, but would appreciate a fixture or a unit test with a .cjs config.

There's also undefined behaviour when both a vocab.config.js and vocab.config.cjs are present. We could do better, but I'm not against merging this PR as-is.

findUp will return the first one it finds, based on the order in the array provided to it (ref). So it's defined, but we could do better by warning when 2 config files are found.

@mrm007
Copy link
Contributor

mrm007 commented Nov 10, 2023

So it's defined, but we could do better by warning when 2 config files are found.

I meant it's undefined for consumers of Vocab. find-up is an implementation detail.

@askoufis askoufis merged commit 8228608 into master Nov 12, 2023
5 checks passed
@askoufis askoufis deleted the add-support-for-vocab-cjs branch November 12, 2023 22:29
@seek-oss-ci seek-oss-ci mentioned this pull request Nov 12, 2023
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.

None yet

3 participants