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

Documentation improvements #415

Closed
nathancarter opened this issue Apr 18, 2023 · 8 comments
Closed

Documentation improvements #415

nathancarter opened this issue Apr 18, 2023 · 8 comments
Assignees

Comments

@nathancarter
Copy link

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

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

hildjj commented Apr 18, 2023

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

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

hildjj commented Apr 19, 2023

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

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! :)

@hildjj hildjj self-assigned this Apr 19, 2023
@hildjj hildjj closed this as completed in 80e3866 Aug 13, 2023
@iulo
Copy link

iulo commented Dec 22, 2023

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

@hildjj
Copy link
Contributor

hildjj commented Dec 22, 2023

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 hildjj reopened this Dec 22, 2023
hildjj added a commit to hildjj/peggy that referenced this issue Jan 27, 2024
@hildjj
Copy link
Contributor

hildjj commented Jan 27, 2024

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.

@hildjj hildjj closed this as completed Jan 27, 2024
hildjj added a commit to hildjj/peggy that referenced this issue Feb 13, 2024
* main: (107 commits)
  Version update.  Check npmignore.  Audit CHANGELOG.md.
  4.0.0
  Update dependencies final time before release.
  Fix indentation in one part of examples/javascript.pegjs.  Fixes peggyjs#445.
  Code review issues
  Add developer docs.  Fixes peggyjs#466.
  Add directories when they don't exist, rather than throwing an error.  Fixes peggyjs#440.
  Update changelog.  Include peggyjs#463 as well.
  Fix peggyjs#379.  Move reportInfiniteRecursion to prepare phase, ensure that it doesn't keep going when it finds an error.
  Typo: 'rutimes'
  Mark IE as explicitly unsupported
  Switch to flat eslint config.  Lint minified output for web compat.
  Code review nits
  Add tests, fix up fromMem to not need gross hack.
  Adds support for running tests against es module output.  Fixes peggyjs#399.
  Slight wording tweak.  Double-checked that peggyjs#415 is fixed.
  Fixes peggyjs#434
  Update bundles as well
  Fixes peggyjs#450
  Update dependencies, including new lint rules.  Move to simpler tsconfig so that eslint can easily find the correct file.  Fix small lint issues with new rules.
  ...
hildjj added a commit that referenced this issue Feb 13, 2024
* main: (107 commits)
  Version update.  Check npmignore.  Audit CHANGELOG.md.
  4.0.0
  Update dependencies final time before release.
  Fix indentation in one part of examples/javascript.pegjs.  Fixes #445.
  Code review issues
  Add developer docs.  Fixes #466.
  Add directories when they don't exist, rather than throwing an error.  Fixes #440.
  Update changelog.  Include #463 as well.
  Fix #379.  Move reportInfiniteRecursion to prepare phase, ensure that it doesn't keep going when it finds an error.
  Typo: 'rutimes'
  Mark IE as explicitly unsupported
  Switch to flat eslint config.  Lint minified output for web compat.
  Code review nits
  Add tests, fix up fromMem to not need gross hack.
  Adds support for running tests against es module output.  Fixes #399.
  Slight wording tweak.  Double-checked that #415 is fixed.
  Fixes #434
  Update bundles as well
  Fixes #450
  Update dependencies, including new lint rules.  Move to simpler tsconfig so that eslint can easily find the correct file.  Fix small lint issues with new rules.
  ...
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

4 participants