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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FeatureRequest] esm release #2079

Open
loynoir opened this issue May 13, 2021 · 26 comments
Open

[FeatureRequest] esm release #2079

loynoir opened this issue May 13, 2021 · 26 comments

Comments

@loynoir
Copy link

loynoir commented May 13, 2021

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
Copy link
Contributor

ariya commented May 13, 2021

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

@loynoir
Copy link
Author

loynoir commented May 13, 2021

Got esm release without rollup plugin. 馃榾

@jogibear9988
Copy link

@jogibear9988
Copy link

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

@jogibear9988
Copy link

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
Copy link
Author

loynoir commented May 19, 2021

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
Copy link
Author

loynoir commented May 19, 2021

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
Copy link
Author

loynoir commented May 19, 2021

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
Copy link

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

@loynoir
Copy link
Author

loynoir commented May 20, 2021

@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
Copy link

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
Copy link

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

@jogibear9988
Copy link

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

@loynoir
Copy link
Author

loynoir commented May 21, 2021

@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
Copy link

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

@loynoir
Copy link
Author

loynoir commented May 21, 2021

TS4053 fixed. Now there is .d.ts. 馃槒

Try it at,

npm install loynoir/esprima

add npm install <gitrepo> support recently.

@lschirmbrand
Copy link

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

@loynoir
Copy link
Author

loynoir commented May 27, 2021

@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
Copy link

@loynoir but we used

 npm install loynoir/esprima

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

@loynoir
Copy link
Author

loynoir commented May 27, 2021

@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
  1. 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
Copy link
Author

loynoir commented May 27, 2021

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

node --version
npm --version

@loynoir
Copy link
Author

loynoir commented May 27, 2021

@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
Copy link

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

@loynoir
Copy link
Author

loynoir commented May 28, 2021

@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
> [email protected] 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
Copy link

jogibear9988 commented Jun 20, 2021

@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
Copy link
Author

loynoir commented Jun 22, 2021

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

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

No branches or pull requests

4 participants