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

add Node.js v20 support #28

Merged
merged 7 commits into from
Jun 2, 2023
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
20 changes: 2 additions & 18 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ jobs:

strategy:
matrix:
node-version: [12.x, 14.x, 16.10.0, 16.16.0, 16.17.0, 16.x, 17.x, 18.5.0, 18.x]
node-version: [12.x, 14.x, 16.10.0, 16.16.0, 16.17.0, 16.x, 17.x, 18.5.0, 18.x, 20.x]
Qard marked this conversation as resolved.
Show resolved Hide resolved

steps:
- uses: actions/checkout@v2
Expand All @@ -23,28 +23,12 @@ jobs:
- run: npm run test:ts
if: (matrix.node-version != '12.x' && matrix.node-version != '14.x' && matrix.node-version != '16.10.0')

build-unsupported:
runs-on: ubuntu-latest

strategy:
matrix:
node-version: [20.x]

steps:
- uses: actions/checkout@v2
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v2
with:
node-version: ${{ matrix.node-version }}
- run: npm install
- run: npm test:unsupported

build-win:
runs-on: windows-latest

strategy:
matrix:
node-version: [ 12.x, 14.x, 16.10.0, 16.16.0, 16.17.0, 16.x, 18.5.0, 18.x ]
node-version: [12.x, 14.x, 16.10.0, 16.16.0, 16.17.0, 16.x, 18.5.0, 18.x, 20.x]

steps:
- uses: actions/checkout@v2
Expand Down
19 changes: 11 additions & 8 deletions hook.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ const NODE_MINOR = Number(NODE_VERSION[1])

let entrypoint

if (NODE_MAJOR >= 20) {
getExports = require('./lib/get-exports.js')
} else {
getExports = (url) => import(url).then(Object.keys)
}

function hasIitm (url) {
try {
return new URL(url).searchParams.has('iitm')
Expand Down Expand Up @@ -103,16 +109,16 @@ function createHook (meta) {

return {
url: addIitm(url.url),
shortCircuit: true
shortCircuit: true,
format: url.format
}
}

const iitmURL = new URL('lib/register.js', meta.url).toString()
async function getSource (url, context, parentGetSource) {
if (hasIitm(url)) {
const realUrl = deleteIitm(url)
const realModule = await import(realUrl)
const exportNames = Object.keys(realModule)
const exportNames = await getExports(realUrl, context, parentGetSource)
return {
source: `
import { register } from '${iitmURL}'
Expand All @@ -137,7 +143,7 @@ register('${realUrl}', namespace, set, '${specifiers.get(realUrl)}')
// For Node.js 16.12.0 and higher.
async function load (url, context, parentLoad) {
if (hasIitm(url)) {
const { source } = await getSource(url, context)
const { source } = await getSource(url, context, parentLoad)
return {
source,
shortCircuit: true,
Expand All @@ -148,10 +154,7 @@ register('${realUrl}', namespace, set, '${specifiers.get(realUrl)}')
return parentLoad(url, context, parentLoad)
}

if (NODE_MAJOR >= 20) {
process.emitWarning('import-in-the-middle is currently unsupported on Node.js v20 and has been disabled.')
return {} // TODO: Add support for Node >=20
} else if (NODE_MAJOR >= 17 || (NODE_MAJOR === 16 && NODE_MINOR >= 12)) {
if (NODE_MAJOR >= 17 || (NODE_MAJOR === 16 && NODE_MINOR >= 12)) {
return { load, resolve }
} else {
return {
Expand Down
97 changes: 97 additions & 0 deletions lib/get-esm-exports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
'use strict'

const { Parser } = require('acorn')
const { importAssertions } = require('acorn-import-assertions');

const acornOpts = {
ecmaVersion: 'latest',
sourceType: 'module'
}

const parser = Parser.extend(importAssertions)

function warn (txt) {
process.emitWarning(txt, 'get-esm-exports')
}

function getEsmExports (moduleStr) {
const exportedNames = new Set()
const tree = parser.parse(moduleStr, acornOpts)
for (const node of tree.body) {
if (!node.type.startsWith('Export')) continue
switch (node.type) {
case 'ExportNamedDeclaration':
if (node.declaration) {
parseDeclaration(node, exportedNames)
} else {
parseSpecifiers(node, exportedNames)
}
break
case 'ExportDefaultDeclaration':
exportedNames.add('default')
break
case 'ExportAllDeclaration':
if (node.exported) {
exportedNames.add(node.exported.name)
} else {
exportedNames.add('*')
}
break
default:
warn('unrecognized export type: ' + node.type)
}
}
return Array.from(exportedNames)
}

function parseDeclaration (node, exportedNames) {
switch (node.declaration.type) {
case 'FunctionDeclaration':
exportedNames.add(node.declaration.id.name)
break
case 'VariableDeclaration':
for (const varDecl of node.declaration.declarations) {
parseVariableDeclaration(varDecl, exportedNames)
}
break
case 'ClassDeclaration':
exportedNames.add(node.declaration.id.name)
break
default:
warn('unknown declaration type: ' + node.delcaration.type)
}
}

function parseVariableDeclaration (node, exportedNames) {
switch (node.id.type) {
case 'Identifier':
exportedNames.add(node.id.name)
break
case 'ObjectPattern':
for (const prop of node.id.properties) {
exportedNames.add(prop.value.name)
}
break
case 'ArrayPattern':
for (const elem of node.id.elements) {
exportedNames.add(elem.name)
}
break
default:
warn('unknown variable declaration type: ' + node.id.type)
}
}

function parseSpecifiers (node, exportedNames) {
for (const specifier of node.specifiers) {
if (specifier.exported.type === 'Identifier') {
exportedNames.add(specifier.exported.name)
} else if (specifier.exported.type === 'Literal') {
exportedNames.add(specifier.exported.value)
} else {
warn('unrecognized specifier type: ' + specifier.exported.type)
}
}
}

module.exports = getEsmExports
51 changes: 51 additions & 0 deletions lib/get-exports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
'use strict'

const getEsmExports = require('./get-esm-exports.js')
const { parse: getCjsExports } = require('cjs-module-lexer')
const fs = require('fs')
const { fileURLToPath } = require('url')

function addDefault(arr) {
return Array.from(new Set(['default', ...arr]))
}

async function getExports (url, context, parentLoad) {
// `parentLoad` gives us the possibility of getting the source
// from an upstream loader. This doesn't always work though,
// so later on we fall back to reading it from disk.
const parentCtx = await parentLoad(url, context)
let source = parentCtx.source
const format = parentCtx.format

// TODO support non-node/file urls somehow?
if (format === 'builtin') {
// Builtins don't give us the source property, so we're stuck
// just requiring it to get the exports.
return addDefault(Object.keys(require(url)))
}

if (!source) {
// Sometimes source is retrieved by parentLoad, sometimes it isn't.
source = fs.readFileSync(fileURLToPath(url), 'utf8')
}

if (format === 'module') {
return getEsmExports(source)
}
if (format === 'commonjs') {
return addDefault(getCjsExports(source).exports)
}

// At this point our `format` is either undefined or not known by us. Fall
// back to parsing as ESM/CJS.
Copy link
Member

Choose a reason for hiding this comment

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

Could it not be wasm? What happens in that case?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK that's not in Node.js 20, so we'll cross that bridge when we come to it.

Copy link
Member

Choose a reason for hiding this comment

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

It's behind a flag, but it is there. There's also json modules.

If I understand right, if we don't return anything from the loader then it will skip it and use the default/built-in logic, correct? So just not handling formats we don't specifically support here is fine?

const esmExports = getEsmExports(source)
if (!esmExports.length) {
// TODO(bengl) it's might be possible to get here if somehow the format
// isn't set at first and yet we have an ESM module with no exports.
// I couldn't construct an example that would do this, so maybe it's
// impossible?
return addDefault(getCjsExports(source).exports)
}
}

module.exports = getExports
10 changes: 6 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@
"description": "Intercept imports in Node.js",
"main": "index.js",
"scripts": {
"test": "c8 --check-coverage --lines 85 imhotap --runner test/runtest --files test/{hook,low-level,other}/*",
"test:unsupported": "imhotap --runner test/runtest --files test/hook/loader.mjs",
"test-win": "c8 --check-coverage --lines 85 imhotap --runner test\\runtest.bat --files test/{hook,low-level,other}/*",
"test": "c8 --check-coverage --lines 85 imhotap --runner test/runtest --files test/{hook,low-level,other,get-esm-exports}/*",
"test-win": "c8 --check-coverage --lines 85 imhotap --runner test\\runtest.bat --files test/{hook,low-level,other,get-esm-exports}/*",
"test:ts": "c8 imhotap --runner test/runtest --files test/typescript/*.test.mts",
"test-win:ts": "c8 imhotap --runner test\\runtest.bat --files test/typescript/*.test.mts",
"coverage": "c8 --reporter html imhotap --runner test/runtest --files test/{hook,low-level,other}/* && echo '\nNow open coverage/index.html\n'"
"coverage": "c8 --reporter html imhotap --runner test/runtest --files test/{hook,low-level,other,get-esm-exports}/* && echo '\nNow open coverage/index.html\n'"
},
"repository": {
"type": "git",
Expand Down Expand Up @@ -37,6 +36,9 @@
"typescript": "^4.7.4"
},
"dependencies": {
"acorn": "^8.8.2",
Copy link
Contributor

@tlhunter tlhunter Jun 2, 2023

Choose a reason for hiding this comment

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

I don't see any output on the PR about the module sizes...

For a naked project, installing these two packages results in 528K of disk space. Doesn't look like any of that would be deduped with existing dependencies since acorn itself has no deps:

λ /tmp/acorn/ npm ls --depth=10
[email protected] /private/tmp/acorn
├─┬ [email protected]
│ └── [email protected] deduped
└── [email protected]

Copy link
Contributor

Choose a reason for hiding this comment

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

Compressed, the files take up 133KB, which would be the approx change to a Lambda bundle.

"acorn-import-assertions": "^1.9.0",
"cjs-module-lexer": "^1.2.2",
"module-details-from-path": "^1.0.3"
}
}
32 changes: 32 additions & 0 deletions test/fixtures/esm-exports.txt
tlhunter marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Exporting declarations
export let name1, name2/*, … */; // also var //| name1,name2
export const name1 = 1, name2 = 2/*, … */; // also var, let //| name1,name2
export function functionName() { /* … */ } //| functionName
export class ClassName { /* … */ } //| ClassName
export function* generatorFunctionName() { /* … */ } //| generatorFunctionName
export const { name1, name2: bar } = o; //| name1,bar
export const [ name1, name2 ] = array; //| name1,name2

// Export list
let name1, nameN; export { name1, /* …, */ nameN }; //| name1,nameN
let variable1, variable2, nameN; export { variable1 as name1, variable2 as name2, /* …, */ nameN }; //| name1,name2,nameN
let variable1; export { variable1 as "string name" }; //| string name
let name1; export { name1 as default /*, … */ }; //| default

// Default exports
export default expression; //| default
export default function functionName() { /* … */ } //| default
export default class ClassName { /* … */ } //| default
export default function* generatorFunctionName() { /* … */ } //| default
export default function () { /* … */ } //| default
export default class { /* … */ } //| default
export default function* () { /* … */ } //| default

// Aggregating modules
export * from "module-name"; //| *
Qard marked this conversation as resolved.
Show resolved Hide resolved
export * as name1 from "module-name"; //| name1
export { name1, /* …, */ nameN } from "module-name"; //| name1,nameN
export { import1 as name1, import2 as name2, /* …, */ nameN } from "module-name"; //| name1,name2,nameN
export { default, /* …, */ } from "module-name"; //| default
export { default as name1 } from "module-name"; //| name1

31 changes: 31 additions & 0 deletions test/get-esm-exports/v20-get-esm-exports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
'use strict'

const getEsmExports = require('../../lib/get-esm-exports.js')
const fs = require('fs')
const assert = require('assert')
const path = require('path')

const fixturePath = path.join(__dirname, '../fixtures/esm-exports.txt')
const fixture = fs.readFileSync(fixturePath, 'utf8')

fixture.split('\n').forEach(line => {
if (!line.includes(' //| ')) return
const [mod, testStr] = line.split(' //| ')
const expectedNames = testStr.split(',').map(x => x.trim())
if (expectedNames[0] === '') {
expectedNames.length = 0
}
const names = getEsmExports(mod)
assert.deepEqual(expectedNames, names)
console.log(`${mod}\n ✅ contains exports: ${testStr}`)
})

// // Generate fixture data
// fixture.split('\n').forEach(line => {
// if (!line.includes('export ')) {
// console.log(line)
// return
// }
// const names = getEsmExports(line)
// console.log(line, '//|', names.join(','))
// })