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

Fixes #329 #362

Merged
merged 2 commits into from
Mar 1, 2023
Merged

Fixes #329 #362

merged 2 commits into from
Mar 1, 2023

Conversation

hildjj
Copy link
Contributor

@hildjj hildjj commented Mar 1, 2023

I'd like someone with more TypeScript intuition to think about how much this breaks things. I realize that it means that we're not going to get as good type checking on known parameters, but this is what we get for having inherited an API.

* 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

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.

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

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.

@hildjj hildjj merged commit a9fe185 into peggyjs:main Mar 1, 2023
@hildjj hildjj deleted the ParserBuildOptions branch March 1, 2023 21:21
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

Successfully merging this pull request may close these issues.

2 participants