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

Non-identifier exports not detected? #47

Closed
evanw opened this issue Mar 12, 2021 · 4 comments
Closed

Non-identifier exports not detected? #47

evanw opened this issue Mar 12, 2021 · 4 comments

Comments

@evanw
Copy link

evanw commented Mar 12, 2021

I came across some unexpected behavior:

const cml = require('cjs-module-lexer')
cml.init().then(() => {
  const result = cml.parse(`module.exports = {fi, if: 0}`)
  console.log(result)
})

Expected:

{ exports: [ 'fi', 'if' ], reexports: [] }

Observed:

{ exports: [ 'fi' ], reexports: [] }

I expected this to work because ESM lets you do this:

import { if as foo } from './example.js'
@evanw
Copy link
Author

evanw commented Mar 12, 2021

Wait, maybe it doesn't have anything to do with keywords. It looks like mapping to null works but mapping to 0 doesn't? Here are some tests with various expressions:

module.exports = {fi, if: 0}
{ exports: [ 'fi' ], reexports: [] }

module.exports = {fi, if: !0}
{ exports: [ 'fi' ], reexports: [] }

module.exports = {fi, if: -0}
{ exports: [ 'fi' ], reexports: [] }

module.exports = {fi, if: (true)}
{ exports: [ 'fi' ], reexports: [] }

module.exports = {fi, if: [true]}
{ exports: [ 'fi' ], reexports: [] }

module.exports = {fi, if: null}
{ exports: [ 'fi', 'if' ], reexports: [] }

module.exports = {fi, if: true}
{ exports: [ 'fi', 'if' ], reexports: [] }

I guess this is because the grammar is specified as (IDENTIFIER (`:` IDENTIFIER )?). Is there any reason to not support arbitrary expressions? Like module.exports = { version: '0.1.0' }.

I will use if: null for now.

@evanw evanw changed the title Keyword exports not detected? Non-identifier exports not detected? Mar 12, 2021
@guybedford
Copy link
Collaborator

@evanw this is just a very simple lexer designed to be small and fast and it doesn't even have an expression parser. We could extend the grammar to support direct primitives in the object form, but for now I'm concerned that may be even more confusing too, it's not a clear line by any means yes. See also #6.

Supporting numbers and strings would be straightforward, I'm just not sure we want to push the blurry line in that direction here, would value your thoughts.

@guybedford
Copy link
Collaborator

The latest update has been upstreamed now, and the object form detection remains the same without string support. I'm open to further suggestions here, although the window is closing on the current set of updates. Closing for now but just let me know if we should reopen or make changes here further.

@evanw
Copy link
Author

evanw commented Mar 18, 2021

it doesn't even have an expression parser

Ah, this makes sense. I was mainly just surprised that support for this wasn't necessary to handle modules like that in the wild.

I recently shipped support for the 0 && module.exports = {...} pattern in esbuild btw. So far so good.

Peeja added a commit to Peeja/SPARQL.js that referenced this issue Jul 14, 2023
Node attempts to make CommonJS exports available as named exports when
imported by ES modules. It does this by statically analyzing the CJS
module using `cjs-module-lexer`. However, as the name implies, it's a
lexer, not a full parser, and it only supports limited common patterns
for building a `module.exports` object. Notably, it supports
`module.exports = { ... }`, but only if the values in that object are
all simple identifiers. Otherwise it will include only the keys up to
the first non-identifier value it finds, and bail after that.

nodejs/cjs-module-lexer#47

That means that the `Generator` and `Wildcard` exports were missing in
ES modules. This change makes them available again.
RubenVerborgh pushed a commit to Peeja/SPARQL.js that referenced this issue Aug 10, 2024
Node attempts to make CommonJS exports available as named exports when
imported by ES modules. It does this by statically analyzing the CJS
module using `cjs-module-lexer`. However, as the name implies, it's a
lexer, not a full parser, and it only supports limited common patterns
for building a `module.exports` object. Notably, it supports
`module.exports = { ... }`, but only if the values in that object are
all simple identifiers. Otherwise it will include only the keys up to
the first non-identifier value it finds, and bail after that.

nodejs/cjs-module-lexer#47

That means that the `Generator` and `Wildcard` exports were missing in
ES modules. This change makes them available again.
RubenVerborgh pushed a commit to Peeja/SPARQL.js that referenced this issue Aug 10, 2024
Node attempts to make CommonJS exports available as named exports when
imported by ES modules. It does this by statically analyzing the CJS
module using `cjs-module-lexer`. However, as the name implies, it's a
lexer, not a full parser, and it only supports limited common patterns
for building a `module.exports` object. Notably, it supports
`module.exports = { ... }`, but only if the values in that object are
all simple identifiers. Otherwise it will include only the keys up to
the first non-identifier value it finds, and bail after that.

nodejs/cjs-module-lexer#47

That means that the `Generator` and `Wildcard` exports were missing in
ES modules. This change makes them available again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants