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.

Sign up for GitHub

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

Jump to bottom

Fixes #329 #362

Merged
merged 2 commits into from
Mar 1, 2023
Merged

Fixes #329 #362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
42a78f8
Fixes #329
hildjj Mar 1, 2023
f31011d
Update changelog
hildjj Mar 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,13 @@ Released: TBD

### Minor Changes

- [#329](https://github.com/peggyjs/peggy/issues/329)] Allow plugin options in
generate. This change loosens type checking strictness to allow for options
unknown to Peggy, but used by plugins such as ts-pegjs. From @hildjj.

### Bug Fixes

- [[#359](https://github.com/peggyjs/peggy/issues/359)] Do not treat as many
- [#359](https://github.com/peggyjs/peggy/issues/359)] Do not treat as many
words as reserved. Clarify the documentation about identifiers. Ensure
that it is more clear that the target language being generated determines
what words are reserved. Clarify that reserved word checking is only
Expand Down
6 changes: 6 additions & 0 deletions lib/peg.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1185,6 +1185,12 @@ export interface BuildOptionsBase {
}

export interface ParserBuildOptions extends BuildOptionsBase {
/**
* Extensions may need to the caller to pass in otherwise-unknown options.
* ts-pegjs has an example in its README.md.
*/
[extensionOpts: string]: any;
Copy link

@reverofevil reverofevil Mar 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't break much, but in general index signatures are not very stable. The correct approach to pass options to plugins looks like this:

import { generate } from 'peggy';
import { goodPlugin } from 'good-plugin';

const code = generate({
    // peggy options
    plugins: [
        goodPlugin({
            // plugin options
        })
    ],
})

A good real-world example is webpack's API vs vite's API. At the very least, there is no guarantee two plugins won't use same top-level key for different purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this would have been the right approach if we were designing the API, but I don't think breaking existing plugins is warranted for this change.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make a plan on how to phase out old API. A good start would be to review this PR. It highlights several problems of typing the existing API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we need a plan, and I'm ok with moving to typescript as a part of a larger breaking change. I think the next step is a discussion of how we generate code (which will cause a massive downstream breakage if we change anything), then figuring out what else we're going to break at the same time. I'm working on writing something up about code generation.

Copy link

@reverofevil reverofevil Mar 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linked PR adds TS without breaking changes (although I didn't test it). The point in reviewing the PR is to get an idea of where types get especially gnarly, and of which things we'd like to avoid. For example, codegen there isn't that pretty either.


/**
* If set to `"parser"`, the method will return generated parser object;
* if set to `"source"`, it will return parser source code as a string;
Expand Down
3 changes: 3 additions & 0 deletions test/types/peg.test-d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ describe("peg.d.ts", () => {

res = parser.parse("buzz\n11\nfizz\n", { start: 10 });
expect(res).toStrictEqual(["buzz", 11, "fizz"]);

res = peggy.generate("foo='a'", { unknown: { more: 12 } });
expectType<peggy.Parser>(parser);
});

it("types SyntaxError correctly", () => {
Expand Down