Skip to content
This repository has been archived by the owner on May 22, 2024. It is now read-only.

fix: only transpile ESM imports when entrypoint is ESM #807

Merged
merged 5 commits into from
Nov 17, 2021
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 16 additions & 9 deletions src/runtimes/node/bundlers/nft/es_modules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,12 @@ const patchESMPackage = async (path: string, fsCache: FsCache) => {
return JSON.stringify(patchedPackageJson)
}

const shouldTranspile = (path: string, cache: Map<string, boolean>, reasons: NodeFileTraceReasons): boolean => {
const shouldTranspile = (
path: string,
cache: Map<string, boolean>,
esmPaths: Set<string>,
reasons: NodeFileTraceReasons,
): boolean => {
if (cache.has(path)) {
return cache.get(path) as boolean
}
Expand All @@ -47,16 +52,18 @@ const shouldTranspile = (path: string, cache: Map<string, boolean>, reasons: Nod

const { parents } = reason

// If the path doesn't have any parents, it's safe to transpile.
// If the path is an entrypoint, we transpile it only if it's an ESM file.
if (parents.size === 0) {
cache.set(path, true)
const isESM = esmPaths.has(path)

return true
cache.set(path, isESM)

return isESM
}

// The path should be transpiled if every parent will also be transpiled, or
// if there is no parent.
const shouldTranspilePath = [...parents].every((parentPath) => shouldTranspile(parentPath, cache, reasons))
const shouldTranspilePath = [...parents].every((parentPath) => shouldTranspile(parentPath, cache, esmPaths, reasons))

cache.set(path, shouldTranspilePath)

Expand All @@ -77,23 +84,23 @@ const transpileESM = async ({
reasons: NodeFileTraceReasons
}) => {
const cache: Map<string, boolean> = new Map()
const paths = [...esmPaths].filter((path) => shouldTranspile(path, cache, reasons))
const pathsSet = new Set(paths)
const pathsToTranspile = [...esmPaths].filter((path) => shouldTranspile(path, cache, esmPaths, reasons))
const pathsToTranspileSet = new Set(pathsToTranspile)
const packageJsonPaths: string[] = [...reasons.entries()]
.filter(([path, reason]) => {
if (basename(path) !== 'package.json') {
return false
}

const needsPatch = [...reason.parents].some((parentPath) => pathsSet.has(parentPath))
const needsPatch = [...reason.parents].some((parentPath) => pathsToTranspileSet.has(parentPath))

return needsPatch
})
.map(([path]) => (basePath ? resolve(basePath, path) : resolve(path)))
const rewrites = await getPatchedESMPackages(packageJsonPaths, fsCache)

await Promise.all(
paths.map(async (path) => {
pathsToTranspile.map(async (path) => {
const absolutePath = basePath ? resolve(basePath, path) : resolve(path)
const transpiled = await transpile(absolutePath, config)

Expand Down
4 changes: 4 additions & 0 deletions tests/fixtures/node-cjs-importing-mjs/function.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
exports.handler = async (event, context) => {
const { App } = await import('./lib/file.mjs')
return new App(event, context)
}
10 changes: 10 additions & 0 deletions tests/fixtures/node-cjs-importing-mjs/lib/file.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
class App {
constructor(event, context) {
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

wait, what? Constructors can return something? this weird

Copy link
Contributor

Choose a reason for hiding this comment

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

but fine for a fixture, I guess ^^

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea was to replicate this scenario, but using a class or not is actually irrelevant for the test case here. All that matters is that the file is ESM.

statusCode: 200,
body: 'Hello world',
}
}
}

export { App }
23 changes: 23 additions & 0 deletions tests/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,29 @@ testMany(
},
)

testMany(
'Can bundle CJS functions that import ESM files with an `import()` expression',
['bundler_esbuild', 'bundler_nft', 'bundler_nft_transpile'],
async (options, t) => {
const fixtureName = 'node-cjs-importing-mjs'
const { files, tmpDir } = await zipFixture(t, fixtureName, {
opts: options,
})

await unzipFiles(files)

const func = require(join(tmpDir, 'function.js'))

// Dynamic imports were added in Node v13.2.0.
if (semver.gte(nodeVersion, '13.2.0')) {
const { body, statusCode } = await func.handler()

t.is(body, 'Hello world')
t.is(statusCode, 200)
}
},
)

testMany(
'Can require local files deeply',
['bundler_default', 'bundler_esbuild', 'bundler_esbuild_zisi', 'bundler_default_nft', 'bundler_nft'],
Expand Down