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 .cjs config files #129

Closed
wants to merge 3 commits into from
Closed

Conversation

guillaumervls
Copy link

@guillaumervls guillaumervls commented Jun 21, 2021

Description

Hello,

This is a quick* fix the use of JS config files in projects where "type": "module" is used in package.json (which will me more and more frequent now that Node 10 obsolete).
Cheers!
*The right way might be to support it via import

Performance impact

None

Security impact

None

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.

(not sure if that's something worth mentioning in README):

  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

Hello,
This is a quick* fix the use of JS config files in projects where `"type": "module"` is used in `package.json` (which will me more and more frequent now that Node 10 obsolete).
Cheers!
*The right way might be to support it via `import`
@benjie
Copy link
Member

benjie commented Jun 28, 2021

Could you complete this with a .mjs option? await import(...)

@guillaumervls
Copy link
Author

I've added it, but I don't know how to do the correct switch between require and import. For now I've put a test on the file extension, but in the case of .js the correct call depends on the type field in package.json (import for module and require otherwise).

src/commands/_common.ts Outdated Show resolved Hide resolved
@benjie
Copy link
Member

benjie commented Jul 2, 2021

in the case of .js the correct call depends on the type field in package.json (import for module and require otherwise).

We could scan for this I suppose. It feels a bit expensive though since we might have to walk up a directory tree looking for a package.json which might not exist (e.g. if you're using migrate standalone). I think just requiring .mjs for now is the least painful option.

Co-authored-by: Benjie Gillam <benjie@jemjie.com>
@guillaumervls
Copy link
Author

Yes. I actually thought there could have been another solution than scanning the file system, some kind of global variable that Node sets when it's in "module" mode, that I wasn't aware of.

@guillaumervls guillaumervls requested a review from benjie July 5, 2021 09:58
@benjie
Copy link
Member

benjie commented Jul 14, 2021

Yes. I actually thought there could have been another solution than scanning the file system, some kind of global variable that Node sets when it's in "module" mode, that I wasn't aware of.

I tried looking for this, but I've not managed to find it either. I have found an interesting behaviour though - in a commonJS file if require.main is undefined then you were started in module mode. This doesn't necessarily mean that this is what package.json states though. To see this:

// foo.mjs
import * as assert from 'assert';

assert.ok(true);
console.log("OK");
await import("./common.cjs");
// foo.cjs
const assert = require('assert');

assert.ok(true);
console.log("OK");
require("./common.cjs");
// common.cjs
console.log("Hello from common");
console.log(require.main);

Now run node foo.mjs and node foo.cjs and spot the difference.

@benjie
Copy link
Member

benjie commented Jul 14, 2021

Wait... Are we overthinking it?

import() can be used from both CJS and MJS files. And it can process both CJS and MJS files. So if import is available and extension is .js we can just import it and Node'll figure it out?

(As you may guess, I'm not keen to merge this until I'm certain the direction we're taking is correct.)

@guillaumervls
Copy link
Author

Hi @benjie,

Sorry for the (very) late reply. I think you're right, I dit it here #140 (I closed this PR by accident)

Cheers !

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