Skip to content
This repository has been archived by the owner on Apr 16, 2020. It is now read-only.

Utility method for detecting ES module syntax #69

Closed

Conversation

GeoffreyBooth
Copy link
Member

@GeoffreyBooth GeoffreyBooth commented Apr 18, 2019

Per #68 (comment)

const { containsModuleSyntax } = require('module');

containsModuleSyntax('import { shuffle } from "lodash"'); // true
containsModuleSyntax('console.log(process.version)'); // false

containsModuleSyntax('import "./file.mjs"'); // true
containsModuleSyntax('import("./file.mjs")'); // false

@MylesBorins
Copy link
Contributor

With a quick audit I noticed that we have a somewhat similar lower level function currently in assert called parseCode

Perhaps we can move that into an internal util function and share the use of acorn between the two places in the code base? I'm going to be honest that I'm not super thrilled about using acorn more in core... and there might be similar push back upstream.

@GeoffreyBooth
Copy link
Member Author

I understand the hesitance to further dependence on Acorn, but the alternative would be to reimplement Acorn’s token parsing, which is even less appealing. I looked at parseCode but it’s specifically only searching for CallExpression nodes, whereas I need to find import and export statements; also that function fully parses the input source code, whereas my version only tokenizes it (and breaks early if it finds import or export). Tokenizing is the stage that happens before parsing, so the function is faster overall if it can do its work without needing to get to the parsing stage.

I guess the question is, do you ever see a day when Node refactors out its dependence on Acorn? Because if so, then yes, we should avoid adding more things to Node core that depend on it; but if not, then depending on a library that’s already in core doesn’t really come at any cost. Besides assert, the REPL also depends on Acorn fairly extensively; it would be quite a challenge to refactor all the places that the REPL relies on it.

@GeoffreyBooth
Copy link
Member Author

Update: @MylesBorins and I spoke offline and we’re not sure if core will object to further dependencies on Acorn, but based on no objections being raised to nodejs/node#27400 we think it’s worth submitting this to core and seeing if that comes up. Especially since the current implementation of this PR isn’t used by Node core anywhere (since it’s no longer coupled with the now-defunct --type) there’s no performance impact on Node.

@GeoffreyBooth GeoffreyBooth force-pushed the detect-import-export branch 2 times, most recently from b635988 to 1ffb68c Compare May 21, 2019 05:51
@GeoffreyBooth
Copy link
Member Author

Upstream PR: nodejs/node#27808

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants