peggyjs / peggy

Peggy: Parser generator for JavaScript
https://peggyjs.org/
MIT License
941 stars 65 forks source link

Documentation improvements #415

Closed nathancarter closed 9 months ago

nathancarter commented 1 year ago

The main documentation page says to import the module using import * as peggy from "peggy"; followed by use like peggy.generate(...) but this gives errors. The correct ES6-style use is actually import peggy from "peggy"; followed by peggy.generate(...).

I would have submitted a PR with these corrections, rather than reporting them, but there is nowhere that tells me how to update the docs. The best guess I have is to directly edit /docs/documentation.html, but that seems like it must be wrong, because /docs/README.md claims that it generates the site using a static site generator. But I'm not sure where the source for that generator would be. So that's a meta-request of this issues: Document how people can contribute to the docs.

reverofevil commented 1 year ago

The correct ES6-style use is actually

No, it's not. ES6 uses that syntax with default export while there is no default export in peggy. Not to mention it's not ES6 module library at all.

hildjj commented 1 year ago

Hm. I just started a new project with npm init -y, then installed peggy with npm i -D peggy, then created a file called t.mjs containing:

import peggy from "peggy"

const g = peggy.generate("foo = '1'")
console.log(g.parse('1'))

which when executed with node t.mjs outputs "1".

import * as peggy from "peggy" fails, since peggy contains something like: { default: { generate: [function] } }

So I think this is a valid complaint.

reverofevil commented 1 year ago

There is no mentions of default in its source code. The environment (node, webpack) can use arbitrary compatibility layers between CommonJS and ES modules (different even under different settings!), and there is no way to know on the CommonJS library side which one is applied by an environment.

Even though documentation starts talking about Node, where *-import is incorrect, further it also mentions a browser, where we can't possibly know it. I think even under ts-node it can work either way with different tsconfigs.

It doesn't seem to me to be a problem of documentation. Probably it would be better to set up a separate ESM entry point in package.json that works for both types of import. As it is right now, this is a CommonJS library, and import issues are completely out of scope.

hildjj commented 1 year ago

In the browser, most people will use the pre-bundled version in browser/peggy.min.js. We could be more careful about recommending that, and we should probably put a browser entry into the package.json pointing to it.

Node's docs say: "When importing CommonJS modules, the module.exports object is provided as the default export"

In Typescript, you do need to do import * as peggy from "peggy" unless you've got allowSyntheticDefaultImports enabled.

All of this could be made clearer in the docs.

By the way, I disagree that es6 users of Peggy are out of scope, but I don't think that matters that much here.

nathancarter commented 1 year ago

The documentation, under the heading JavaScript API, literally says you can use a require-style or import-style way to load the module. If one of the two is out of scope, the docs should make that clear rather than recommending the reader to do what's not supported.

More importantly, doing what's in the docs fails with an error. So regardless of the official position on ES6, the docs currently recommend writing code that will fail in its first 2 lines. (To be specific, the import line doesn't fail, but the next line of code that builds a parser fails because the import didn't bring in the kind of thing that was intended.)

Finally, if you make the change to the docs that I suggested in my original issue report, then everything actually works correctly thereafter. So I don't think there's any reason to shy away from ES6 support, since...well, since you have it already! :)

iulo commented 10 months ago

@hildjj Hi, am I missing something? The docs page still show import * as

hildjj commented 10 months ago

I thought I had fixed this, but maybe we haven't done a release since then. Going to re-open until I double-check.

hildjj commented 9 months ago

I just checked, and this is fixed in head. Made a slight addition to the wording. Will show up on the site once 4.0 is released.