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

Improve CJS/ESM interoperability support #345

Merged
merged 6 commits into from
Jan 22, 2022
Merged
Show file tree
Hide file tree
Changes from 4 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
8 changes: 4 additions & 4 deletions .npmignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
!README.md
!SECURITY.md

!dist/index.cjs
!dist/index.d.ts
!dist/index.js
!dist/middlewares/**/*.d.ts
!dist/cjs/index.js
!dist/cjs/package.json
!dist/esm/index.js
!dist/types/**/*.d.ts
52 changes: 39 additions & 13 deletions bin/build-helmet.js
Original file line number Diff line number Diff line change
@@ -1,42 +1,68 @@
#!/usr/bin/env node
import { promises as fs } from "fs";
import * as path from "path";
import { fileURLToPath } from "url";
import rollupTypescript from "@rollup/plugin-typescript";
import { writeRollup, withCommonJsFile } from "./helpers.js";
import {
writeRollup,
withCommonJsFile,
withEsmFile,
renameFile,
finalizeCommonJs,
moveDir
} from "./helpers.js";

const thisPath = fileURLToPath(import.meta.url);
const rootPath = path.join(path.dirname(thisPath), "..");
const distPath = path.join(rootPath, "dist");
const esmSourcePath = path.join(rootPath, "index.ts");
const esmDistPath = path.join(rootPath, "dist", "index.js");
const commonJsDistPath = path.join(rootPath, "dist", "index.cjs");
const esmDistDir = path.join(distPath, "esm");
const esmDistPath = path.join(esmDistDir, "index.js");
const commonJsDistDir = path.join(distPath, "cjs");
const commonJsDistPath = path.join(commonJsDistDir, "index.js");
const typesDistDir = path.join(distPath, "types");

const compileEsm = () =>
writeRollup(
{
input: esmSourcePath,
plugins: [rollupTypescript({ tsconfig: "./tsconfig-esm.json" })],
},
{ file: esmDistPath }
withEsmFile(esmSourcePath, (esmTempPath) =>
writeRollup(
{
input: esmTempPath,
plugins: [rollupTypescript({ tsconfig: "./tsconfig-esm.json" })],
},
{ file: esmDistPath }
)
);

const compileCommonjs = () =>
withCommonJsFile(esmSourcePath, (commonJsSourcePath) =>
withCommonJsFile(esmSourcePath, (commonJsTempPath) =>
writeRollup(
{
input: commonJsSourcePath,
input: commonJsTempPath,
plugins: [rollupTypescript({ tsconfig: "./tsconfig-commonjs.json" })],
},
{
exports: "default",
exports: "named",
file: commonJsDistPath,
format: "cjs",
}
)
).then(async () => await finalizeCommonJs(commonJsDistDir, esmDistDir))
MitchellCash marked this conversation as resolved.
Show resolved Hide resolved
);

const finalizeTypes = async () => {
await fs.mkdir(typesDistDir);
await fs.mkdir(path.join(typesDistDir, "middlewares"));
MitchellCash marked this conversation as resolved.
Show resolved Hide resolved
await renameFile(
path.join(esmDistDir, "tmp-esm-index.d.ts"),
path.join(distPath, "types", "index.d.ts")
);
await moveDir(path.join(esmDistDir, "middlewares"), path.join(typesDistDir, "middlewares"))
Copy link
Member

Choose a reason for hiding this comment

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

Why can't we just fs.rename this? Why do we need this fancy helper?

Copy link
Contributor Author

@MitchellCash MitchellCash Jan 20, 2022

Choose a reason for hiding this comment

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

fs.rename doesn't handle moving files to directories that don't exist, nor have any sort of recursive renaming AFAIK. So this function was created to recursively traverse a directory tree and create required directories before moving a file.

However, in commit c8e887c I've removed this function in an attempt to simplify things.

Instead of outputting the *.d.ts type files into dist/esm/* and then using the moveDir helper to get them over to dist/types/*. Now when outputting ESM I output the types straight to dist/types/* and then only need to worry about moving dist/index.js to dist/esm/index.js.

We get to the same place as before, but with less code for us to maintain and a little more readable as we only are moving a single file instead of traversing a directory tree moving many files. Happy for feedback!

await fs.rmdir(path.join(esmDistDir, "middlewares"));
}

async function main() {
await compileEsm();
await compileCommonjs();
await finalizeTypes();
}

main(process.argv).catch((err) => {
Expand Down
75 changes: 68 additions & 7 deletions bin/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,84 @@ export async function writeRollup(inputOptions, outputOptions) {
await bundle.close();
}

export async function withEsmFile(esmSourcePath, fn) {
const esmTempPath = path.join(
path.dirname(esmSourcePath),
"tmp-esm-index.ts"
);

const lines = (await fs.readFile(esmSourcePath, "utf8")).split(/\r?\n/);
const startLine = lines.findIndex((line) =>
line.includes("!helmet-start-of-commonjs-exports")
);
const endLine = lines.findIndex((line) =>
line.includes("!helmet-end-of-commonjs-exports")
);
const lineCount = endLine - startLine + 1;
lines.splice(startLine, lineCount);

try {
await fs.writeFile(esmTempPath, lines.join("\n"));

await fn(esmTempPath);
} finally {
await fs.unlink(esmTempPath);
}
}

export async function withCommonJsFile(esmSourcePath, fn) {
const commonJsSourcePath = path.join(
const commonJsTempPath = path.join(
path.dirname(esmSourcePath),
"tmp-commonjs-index.ts"
);

const lines = (await fs.readFile(esmSourcePath, "utf8")).split(/\r?\n/);
const resultLines = lines.slice(
0,
lines.findIndex((line) => line.includes("!helmet-end-of-commonjs"))
const startLine = lines.findIndex((line) =>
line.includes("!helmet-start-of-commonjs-exports")
);
const endLine = lines.findIndex((line) =>
line.includes("!helmet-end-of-commonjs-exports")
);
lines.splice(startLine, 1);
const resultLines = lines.slice(0, endLine);

try {
await fs.writeFile(commonJsSourcePath, resultLines.join("\n"));
await fs.writeFile(commonJsTempPath, resultLines.join("\n"));

await fn(commonJsSourcePath);
await fn(commonJsTempPath);
} finally {
await fs.unlink(commonJsSourcePath);
await fs.unlink(commonJsTempPath);
}
}

export async function renameFile(oldPath, newPath) {
await fs.rename(oldPath, newPath)
.catch((error) => {
if (error) console.error(error);
});
}
Copy link
Member

Choose a reason for hiding this comment

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

It seems strange that we'd drop errors here—I think I'd like it to crash. Could we delete this function and just use fs.rename?


export async function finalizeCommonJs(commonJsDistDir) {
const cjsPackageJson = JSON.stringify({
type: "commonjs",
});

await fs.writeFile(
path.join(commonJsDistDir, "package.json"),
cjsPackageJson
);
}

export async function moveDir(oldPath, newPath) {
await fs.mkdir(newPath, { recursive: true });
let entries = await fs.readdir(oldPath, { withFileTypes: true });

for (let entry of entries) {
let srcPath = path.join(oldPath, entry.name);
let destPath = path.join(newPath, entry.name);

entry.isDirectory() ?
(await moveDir(srcPath, destPath), await fs.rmdir(srcPath)) :
(await fs.copyFile(srcPath, destPath), await fs.rm(srcPath));
}
}
5 changes: 4 additions & 1 deletion index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,10 @@ const helmet: Helmet = Object.assign(

export default helmet;

// !helmet-end-of-commonjs
// !helmet-start-of-commonjs-exports
exports = helmet;
module.exports = helmet;
// !helmet-end-of-commonjs-exports
EvanHahn marked this conversation as resolved.
Show resolved Hide resolved

export {
contentSecurityPolicy,
Expand Down
11 changes: 6 additions & 5 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,14 @@
},
"license": "MIT",
"type": "module",
"types": "./dist/index.d.ts",
"main": "./dist/index.cjs",
"main": "./dist/cjs/index.js",
"module": "./dist/esm/index.js",
"types": "./dist/types/index.d.ts",
"exports": {
".": {
"import": "./dist/index.js",
"require": "./dist/index.cjs",
"types": "./dist/index.d.ts"
MitchellCash marked this conversation as resolved.
Show resolved Hide resolved
"import": "./dist/esm/index.js",
"require": "./dist/cjs/index.js",
"types": "./dist/types/index.d.ts"
}
}
}
5 changes: 2 additions & 3 deletions tsconfig-esm.json
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
{
"extends": "./tsconfig",
"exclude": ["dist"],
"compilerOptions": {
"outDir": "dist",
"declaration": true,
"declarationDir": "."
}
},
"include": ["tmp-esm-index.ts"]
}
6 changes: 4 additions & 2 deletions tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
"noUnusedParameters": true,
"noUncheckedIndexedAccess": true,
"strict": true,
"target": "es6"
}
"target": "es6",
"outDir": "."
},
"exclude": ["node_modules", "bin", "dist"]
}