Skip to content
This repository has been archived by the owner on Sep 2, 2023. It is now read-only.

Table source type detection #465

Merged
merged 1 commit into from
Jan 15, 2020

Conversation

GeoffreyBooth
Copy link
Member

So eight months ago I opened nodejs/node#27808 to add a utility method to Node core for detecting if JavaScript source code contained ES module syntax. That PR is still open and it’s not looking likely that it will be merged anytime soon, so I’m thinking of closing it. I might release the code as a standalone NPM package.

The PR’s containsModuleSyntax method wasn’t all that useful on its own, and wasn’t used by Node core; it was created to be a dependency for potential Node core features like --input-type=auto. Are will still planning on implementing --input-type=auto, or any other feature that might rely on containsModuleSyntax?

If not, I can close the PR and mark this roadmap item as done/tabled, unless anyone has any objections or reasons we might want source detection in the future.

@DerekNonGeneric
Copy link
Contributor

I love the idea. Though I think the problem was the dependence on Acorn.

I hope to compare notes from what I understand.

As you know, I was using containsModuleSyntax in my loader to perform "auto-detection," which has now taken on a new meaning from the perspective of loaders (esp. ones that do not use the builtin resolver). Without containsModuleSyntax, rather than being detected via the source code (keywords, etc.), it's being inferred based on other factors. For example file extension, parent package.json's type field, and so on.

From the perspective of eval, where other factors may be absent, it does seem like --input-type=auto would need something like containsModuleSyntax. If it doesn't make it into core, maybe a drop-in replacement CLI tool possibly dubbed autoeval-node would be necessary.

@GeoffreyBooth
Copy link
Member Author

I think the problem was the dependence on Acorn.

I'm not sure that was an issue, since Acorn is already part of Node core.

From the perspective of eval, where other factors may be absent, it does seem like --input-type=auto would need something like containsModuleSyntax.

I could redo the PR to implement --input-type=auto, to give Node a reason to need this; the containsModuleSyntax could even be kept private if the core maintainers wanted.

Regardless of this method and whether or not it is publicly available, how would people react to a potential PR for --input-type=auto? Where auto treats the input as ESM if it containsModuleSyntax per the logic of that method, and as CommonJS otherwise. It would only apply to string input, as --input-type only applies to strings/piped input.

@ljharb
Copy link
Member

ljharb commented Jan 12, 2020

As i recall, the pushback (mine included) was primarily around auto. There is ambiguity in the Script and Module syntax, and unless there’s no way for an ambiguous file to be mis-parsed, it doesn’t seem appropriate to have that kind of risk in node core.

I think that regardless of the plan in core, it seems more productive to publish and integrate on an npm package first - that way the semantics and edge cases can be fleshed out in advance.

@GeoffreyBooth
Copy link
Member Author

Going to merge this per 2020-01-15 meeting. We can always revisit or reopen, or open new/similar issues.

@GeoffreyBooth GeoffreyBooth merged commit ecbb539 into nodejs:master Jan 15, 2020
@GeoffreyBooth GeoffreyBooth deleted the table-detection branch January 15, 2020 21:06
@GeoffreyBooth
Copy link
Member Author

After this the only two remaining open roadmap items are both in Stage 4, and they’re dual packages/conditional exports and loaders. The former we’re hopefully about to wrap up, the latter I guess we could change the status to say it will be open/iterated upon indefinitely, something like that? I feel like loaders might never be “done” per se.

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

Successfully merging this pull request may close these issues.

3 participants