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

Fix #92: Drop support for useless optimize option #147

Merged
merged 1 commit into from
May 4, 2021

Conversation

Mingun
Copy link
Member

@Mingun Mingun commented May 4, 2021

There no reasons for use value other than speed.

CLI option and definition in .d.ts are left for backward compatibility

Copy link
Contributor

@hildjj hildjj left a comment

Choose a reason for hiding this comment

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

I was assuming we would go ahead and rip the functionality out of generate-js also?

README.md Outdated Show resolved Hide resolved
bin/peggy Show resolved Hide resolved
…reasons for use value other than `speed`
@hildjj
Copy link
Contributor

hildjj commented May 4, 2021

This looks good. One last thing to discuss. Should we issue a DeprecationWarning for people that use "size" through the API? It would be nice, but I don't want to have to mock process.emitWarning in the web build.

@Mingun
Copy link
Member Author

Mingun commented May 4, 2021

Not enough @deprecated comment in the .d.ts? I think, that modern users would, at least, use typescript types and should see that warning.

@hildjj
Copy link
Contributor

hildjj commented May 4, 2021

I'm more worried about the non-modern users here, I think, but I see your point. Let's land this then be on the lookout for issues that get filed if people are confused. I think we should land this now since I want to rebase other changes on top of it. Make sense?

@Mingun
Copy link
Member Author

Mingun commented May 4, 2021

Make sense.

@hildjj hildjj merged commit 10bc9d4 into peggyjs:main May 4, 2021
@Mingun Mingun deleted the drop-size-mode branch May 4, 2021 16:51
@StoneCypher
Copy link
Contributor

Not enough @deprecated comment in the .d.ts? I think, that modern users would, at least, use typescript types and should see that warning.

This isn't how typescript works.

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.

3 participants