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

feat: support CJS configs in ES packages #553

Merged
merged 2 commits into from Jul 12, 2023
Merged

Conversation

quisido
Copy link
Contributor

@quisido quisido commented Feb 23, 2022

Problem statement

When writing an application with "type": "module" in package.json, it becomes impossible to set the NYC config with JS, since it only supports the .js extension and it only supports CommonJS. As a result, any configuration in nyc.config.js will fail for trying to require an ES module.

This PR adds support for nyc.config.cjs, allowing packages with "type": "module" to still configure a nyc.config.cjs file for a CommonJS configuration.

@quisido
Copy link
Contributor Author

quisido commented Jul 5, 2023

Sorry for direct pings:

@astone123 - I see you have merged pull requests before.
@emilyrohrbough - You are the last member of the organization that I see with a commit.

This PR has been out for well over a year now. Any chance it can get reviewed?

@nagash77
Copy link
Contributor

nagash77 commented Jul 6, 2023

Hi @CharlesStover , sorry for the VERY slow response! This PR must have gotten lost in the shuffle. We have since implemented a more robust triage process to make sure issues do not sit like that going forward. I will have the team discuss this PR and see how we would like to proceed.

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

This seems fine - it looks like you've followed the existing pattern, so I don't see any blocker on merging this.

@lmiller1990
Copy link
Contributor

I'd generally like to see a test, but we haven't got any of the other permutations - I'm not sure how much work it would be to add some tests around the various module formats, but since this follows and existing pattern, I think we can merge this up as is. CI is green, so I'm reasonably confident. Let me find one more reviewer.

@CLAassistant
Copy link

CLAassistant commented Jul 9, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ CharlesStover
❌ lmiller1990
You have signed the CLA already but the status is still pending? Let us recheck it.

@lmiller1990
Copy link
Contributor

Hey @CharlesStover, just waiting on our infosec team to update the Snyk API key so the scan can run and we can ship this. I'm sorry for the delay.

@lmiller1990 lmiller1990 changed the title support CJS configs in ES packages feat: support CJS configs in ES packages Jul 12, 2023
@lmiller1990 lmiller1990 merged commit 8015401 into cypress-io:master Jul 12, 2023
22 checks passed
@lmiller1990
Copy link
Contributor

YES!!!! 🚀

@cypress-app-bot
Copy link

🎉 This PR is included in version 3.11.0 🎉

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants