jquery / esprima

ECMAScript parsing infrastructure for multipurpose analysis
http://esprima.org
BSD 2-Clause "Simplified" License
7.04k stars 786 forks source link

[FeatureRequest] esm release #2079

Open loynoir opened 3 years ago

loynoir commented 3 years ago

Will esprima release support es module?

Something like tree-shaking-able dist/esprima.mjs .

ramda/ramda publish cjs and esm at same time, its package.json might helpful.

Last, appreciate a lot for this great library. 😀

ariya commented 3 years ago

Great @loynoir! Would you be willing to come up with a PR (and an explanation on how to test it)?

loynoir commented 3 years ago

Got esm release without rollup plugin. 😀

jogibear9988 commented 3 years ago

https://nodejs.org/api/packages.html#packages_conditional_exports

jogibear9988 commented 3 years ago

@loynoir oh, you already used the cond. exports. :-)

jogibear9988 commented 3 years ago

but should we really use .mjs file extension? I think some webservers would not correctly serve the .mjs files with correct mime type.

See also here: https://developer.mozilla.org/de/docs/Web/JavaScript/Guide/Modules#aside_%E2%80%94_.mjs_versus_.js

loynoir commented 3 years ago

I think .mjs is the only option to make dual module works.

$ node --experimental-repl-await
Welcome to Node.js v14.16.0.
Type ".help" for more information.
> require('.')
'cjs'
> await import('./x.mjs')
[Module: null prototype] { default: 'esm' }
> await import('./x.esm.js')
(node:1726626) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.

add that "type": "module" exclusive statement.

$ node --experimental-repl-await
Welcome to Node.js v14.16.0.
Type ".help" for more information.
> await import('./x.esm.js')
[Module: null prototype] { default: 'esm' }
> await import('./x.mjs')
[Module: null prototype] { default: 'esm' }
> require('.')
Uncaught:
Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: 
loynoir commented 3 years ago

Work: .mjs

  "exports": {
    ".": {
      "require": "./x.js",
      "import": "./x.mjs",
      "default": "./x.js"
    }
  },
> require('rjfxyrkr3z')
'cjs'
> await import('rjfxyrkr3z')
[Module: null prototype] { default: 'esm' }

Not work .esm.js

  "exports": {
    ".": {
      "require": "./x.js",
      "import": "./x.esm.js",
      "default": "./x.js"
    }
  },
> require('rjfxyrkr3z')
'cjs'
> await import('rjfxyrkr3z')
(node:1732077) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
loynoir commented 3 years ago

As mention above, .mjs seems the only option for dual module.

As it is dual module, and for backward compact, I wrote default condition as umd over esm.

So, it should be 100% backward compact if using require("esprima").

So, it should be 100% backward compact if webservers access esprima in a require way. @jogibear9988

So, it should be tree shakable if using import { foobar } from "esprima"

jogibear9988 commented 3 years ago

maybe ".js" only works when you set "type=module " in package.json

loynoir commented 3 years ago

@jogibear9988

To support esprima.esm.js, need declare "type=module" in package.json.

// loading from `esprima.js`
const { parse } = require('esprima')
// Error [ERR_REQUIRE_ESM]: Must use import to load ES Module
// 0% backward compact.

// loading from `esprima.esm.js`, tree shakable
import { parse } from 'esprima'
// OK

So, I prefer esprima.mjs.

// loading from `esprima.js`
const { parse } = require('esprima')
// OK, 100% backward compact

// loading from `esprima.mjs`, tree shakable
import { parse } from 'esprima'
// also OK
jogibear9988 commented 3 years ago

i think .cjs would work for the old files if you use type=module. I would more like to use type=module cause I'm not using a minifier in my project's, and I also think it's time to drop support for old module loaders. But thats only my personal opinion

jogibear9988 commented 3 years ago

but .mjs for esm modules would still be better than no esm version :-)

jogibear9988 commented 3 years ago

maybe we should also include .d.ts type infos in the esm version?

loynoir commented 3 years ago

@jogibear9988

Tried to let tsc generate .d.ts, I have no idea what is going on, better using types from @types/esprima

src/tokenizer.ts:122:5 - error TS4053: Return type of public method from exported class has or is using name 'Error' from external module "<foobar>/esprima/src/error-handler" but cannot be named.

122     errors() {
        ~~~~~~

Found 1 error.
jogibear9988 commented 3 years ago

I don't think that would be better... they would always be older.... Maybe I'll find time to take a look

loynoir commented 3 years ago

TS4053 fixed. Now there is .d.ts. 😏

Try it at,

npm install loynoir/esprima

add npm install <gitrepo> support recently.

lschirmbrand commented 3 years ago

We tried to use your package, but the dist folder is empty

loynoir commented 3 years ago

@lschirmbrand Yes.

jquery/esprima git repo keep dist folder empty, and publish to npm with non-empty dist folder, so I somehow respect that way.

But, I add prepare to npm scripts, which will be autorun by npm on up to date archlinux.

If your npm not support that way, you might clone my fork, and manually run

npm run test
lschirmbrand commented 3 years ago

@loynoir but we used

 npm install loynoir/esprima

this is a npm package? (or am I wrong?)

loynoir commented 3 years ago

@lschirmbrand Sorry. Haven't publish to npm. It is on github.

If you are using

  1. Bleeding edge new npm

    npm install loynoir/esprima
    // npm expand it to
    npm install https://github.com/loynoir/esprima
  2. If you are not using bleeding edge new npm maybe try this

    git clone https://github.com/loynoir/esprima
    cd esprima
    npm install
    npm run test
loynoir commented 3 years ago

@lschirmbrand Could you show some toolchain version? Maybe I could reproduce within a docker.

node --version
npm --version
loynoir commented 3 years ago

@lschirmbrand ./dist on https://github.com/loynoir/esprima-es-dist in case of you got stuck. But I might delete https://github.com/loynoir/esprima-es-dist in the future.

jogibear9988 commented 3 years ago

@loynoir im working with @lschirmbrand we don't use npm, we use yarn. but that shouldn't be a problem?

loynoir commented 3 years ago

@jogibear9988 yarn install original repo DID NOT pass the test. So in my fork I forced testing while you are running install.

$ git clone https://github.com/jquery/esprima 
$ cd esprima
$ yarn install && echo OK
OK
$ yarn test
> esprima@4.0.0-dev lint-code                                                                                    
> eslint src/*.ts                                                                                                

esprima/src/assert.ts                                                                        
  0:0  error  Parsing error: Not implemented                                                                     

esprima/src/character.ts                                                                     
  0:0  error  Parsing error: Not implemented                                                                     

esprima/src/comment-handler.ts                                                               
  0:0  error  Parsing error: Not implemented                                                                     

esprima/src/error-handler.ts                                                                 
  0:0  error  Parsing error: Not implemented                                                                     

So, I don't know the reason, but package manager seems matter in this case.

Maybe related to another thing I found. Forgot to fill an issue.

jogibear9988 commented 3 years ago

@loynoir i've now merged your esm patch in my fork https://github.com/node-projects/esprima-next but i've moved the esm release in an extra folder, with .js extension

loynoir commented 3 years ago

@jogibear9988 Oh, that's good to hear too. 👏