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 CoffeeScript CJS + ESM #3

Merged
merged 5 commits into from
Apr 3, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 7 additions & 17 deletions coffeescript-loader/loader.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import { readFile } from 'fs/promises';
import { readFileSync } from 'fs';
import { createRequire } from 'module';
import { dirname, extname, resolve as resolvePath } from 'path';
import { fileURLToPath, pathToFileURL } from 'url';

import CoffeeScript from 'coffeescript';

const coffeeCompile = (source, filename) => CoffeeScript.compile(source, {
bare: true,
filename
});

const baseURL = pathToFileURL(process.cwd() + '/').href;

Expand Down Expand Up @@ -51,10 +53,7 @@ export async function load(url, context, defaultLoad) {
const { source: rawSource } = await defaultLoad(url, { format });
// This hook converts CoffeeScript source code into JavaScript source code
// for all imported CoffeeScript files.
const transformedSource = CoffeeScript.compile(rawSource.toString(), {
bare: true,
filename: url,
});
const transformedSource = coffeeCompile(rawSource.toString(), url)

return {
format,
Expand Down Expand Up @@ -84,15 +83,6 @@ async function getPackageType(url) {
return dir.length > 1 && getPackageType(resolvePath(dir, '..'));
}


// Register CoffeeScript to also transform CommonJS files. This can more
// thoroughly be done for CoffeeScript specifically via
// `CoffeeScript.register()`, but for purposes of this example this is the
// simplest method.
// Register CoffeeScript to also transform CommonJS files.
const require = createRequire(import.meta.url);
['.coffee', '.litcoffee', '.coffee.md'].forEach(extension => {
require.extensions[extension] = (module, filename) => {
const source = readFileSync(filename, 'utf8');
return CoffeeScript.compile(source, { bare: true, filename });
}
})
require("coffeescript/register")
15 changes: 4 additions & 11 deletions coffeescript-loader/test.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
import { ok } from 'assert';
import { spawn } from 'child_process';
import { execPath } from 'process';
import { fileURLToPath, URL } from 'url';


// Run this test yourself with debugging mode via:
// node --inspect-brk --experimental-loader ./loader.js ./fixtures/esm-and-commonjs-imports.coffee

const child = spawn(execPath, [
'--experimental-loader',
fileURLToPath(new URL('./loader.js', import.meta.url).href),
fileURLToPath(new URL('./fixtures/esm-and-commonjs-imports.coffee', import.meta.url).href),
'./loader.js',
'./fixtures/esm-and-commonjs-imports.coffee',
]);
let stdout = '';
child.stdout.setEncoding('utf8');
Expand All @@ -24,12 +22,7 @@ child.stderr.on('data', (data) => {
});

child.on('close', (code, signal) => {
ok(stdout.includes('Hello from CoffeeScript', 'Main entry transpiles'));
ok(stdout.includes('Hello from CoffeeScript'), 'Main entry transpiles');
ok(stdout.includes('HELLO FROM ESM'), 'ESM import transpiles');

// There is a known issue between ESM + CJS where CJS named exports get lost:
// require.extentions appropriately supplies (transformed) source, but an
// empty object is returned for the module's exports.
// the import does NOT hit ESMLoader or its CJS strategy.
ok(!stdout.includes('Hello from CommonJS!'), 'Named CommonJS import fails');
ok(stdout.includes('Hello from CommonJS!'), 'Named CommonJS import fails');
Copy link
Member

Choose a reason for hiding this comment

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

Does this pass now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does

Copy link
Member

Choose a reason for hiding this comment

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

And what did the trick was adding module._compile, it looks like?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes adding module._compile was the critical piece. coffeescript/register called it in their implementation so I looked to see what was different here.

});