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: cjs support in custom middleware file names #80

Merged
merged 3 commits into from
Mar 20, 2024

Conversation

valoricDe
Copy link
Contributor

@valoricDe valoricDe commented Dec 20, 2023

With the new field "type": "module", in package.json node js might require different file names.

Therefore I added logic that it also handles .cjs and .mjs files

@valoricDe
Copy link
Contributor Author

@olegdovger Hi, this is currently blocking us. Would you be able to support here and review this small PR?

@valoricDe
Copy link
Contributor Author

@Daanoz or @valeriiudodov are you able to help here?

@muratcorlu
Copy link
Owner

muratcorlu commented Jan 17, 2024

Hello. Thanks for your contribution! I realized that I'm not running tests on pipelines. While I adding tests to PR actions, can you add test cases and test on your local (by running npm test)

@riverrun-git
Copy link

riverrun-git commented Mar 18, 2024

@muratcorlu Insisting on additional test cases here is difficult. You would have to duplicate every GET.js, , POST.js, etc. with almost identical files named GET.cjs, GET.mjs, POST.cjs, POST.mjs, etc... Then run the tests in an environment setup to require CJS files to be named *.cjs, then in one where MJS files need to be named *.mjs and then in one where files need to be named *.js.
How to achieve that in a test pipeline is unclear.
This is going to be a blocker for us soon.
Could you not review the (trivial) changes instead?

@muratcorlu
Copy link
Owner

I just tried to write test and realized that, if you write the custom middleware in ESM format like below, current implementation doesn't work, since the library is in commonjs format and we can not just "require" an mjs file.

export default function (req, res) {
  res.json({
    result: 'mjs works'
  });
}

I get error:

SyntaxError: Unexpected token 'export'

So now I'm curious if you managed to run this in your -manual- test? Do you compile mjs files to commonjs before using with connect-api-mocker?

@valoricDe
Copy link
Contributor Author

I moved everything to cjs because of this reason

@muratcorlu
Copy link
Owner

Would it be acceptable to modify this pull request to exclusively support CommonJS and eliminate support for .mjs files, enabling us to proceed with the merge?

Regarding .mjs, we could contemplate transitioning this library to ECMAScript Modules (ESM) to facilitate support for both module systems. This approach is preferable because importing CommonJS files into ESM modules is straightforward, whereas importing .mjs files into CommonJS requires additional effort. Besides, ESM represents the future direction of JavaScript module systems.

index.js Outdated

const methodFiles = [jsMockFile, staticMockFile, wildcardJsMockFile, wildcardStaticMockFile];
const fileExtensions = [methodFileExtension, 'mjs', 'cjs', 'js']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
const fileExtensions = [methodFileExtension, 'mjs', 'cjs', 'js']
const fileExtensions = [methodFileExtension, 'cjs', 'js']

index.js Outdated
@@ -12,6 +12,10 @@ function escapeRegExp(str) {
return str.replace(/[\-\[\]\/\{\}\(\)\*\+\?\.\\\^\$\|]/g, '\\$&');
}

function isJsFile(str) {
return !!str.match(/\.(c|m)?js$/)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
return !!str.match(/\.(c|m)?js$/)
return !!str.match(/\.c?js$/)

@valoricDe
Copy link
Contributor Author

Feel free to commit my suggestions

@muratcorlu muratcorlu changed the title Add mjs and cjs support feat: cjs support in custom middleware file names Mar 20, 2024
@muratcorlu muratcorlu merged commit 65c1513 into muratcorlu:master Mar 20, 2024
1 check passed
Copy link

🎉 This PR is included in version 1.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.

3 participants