Skip to content

Fix panic for top level if and else if#19

Open
RuairidhWilliamson wants to merge 3 commits intowilsonzlin:masterfrom
RuairidhWilliamson:master
Open

Fix panic for top level if and else if#19
RuairidhWilliamson wants to merge 3 commits intowilsonzlin:masterfrom
RuairidhWilliamson:master

Conversation

@RuairidhWilliamson
Copy link
Copy Markdown

@RuairidhWilliamson RuairidhWilliamson commented Jan 28, 2024

Fixes #18

Hello,

This fixes two issues I found after upgrading to 0.6.0

  • When using an if statement in the global scope searching for the closure scope would fail and unwrap None. I fixed this by falling back to the current scope. This address Panic: "called Option::unwrap() on a None value" #18
  • When using an else if statement the assertion always on line 422 always fails. From the comment I think the logic should be inverted.

I believe this will also fix the failed benchmark action on the 0.6.0 release.

Thanks for making this crate :)

@greengeckoz
Copy link
Copy Markdown

I just implemented my own fix for this over here, https://github.com/ddotthomas/minify-js/commit/0443ef828e483f99dc4748d8d8eae4c3af80b21c

I was having trouble understanding parts of the crate diving in but I thought the assert was checking that if an if statement 'returned' before that it should still be returning after being minified. Simply changing the && operator to an == one made the crate work for our project.

Comment thread rust/src/minify/pass1.rs Outdated
Co-authored-by: ddotthomas <[email protected]>
@zachmmeyer
Copy link
Copy Markdown

Any updates on this?

@greengeckoz
Copy link
Copy Markdown

@wilsonzlin are you still active? Hope you're doing okay.

@fxwiegand
Copy link
Copy Markdown

@wilsonzlin Also hope you’re doing well! Since this crate, along with minify-html, seems unmaintained for a while, would you consider moving them into a new minify organization? I’d be happy to help maintain them, and I’m sure other community members would be willing as well.

@neon-mmd
Copy link
Copy Markdown

@RuairidhWilliamson or anyone interested.
(Btw, sorry for the ping)
Seeing the slow and unresponsive behaviours from the maintainers of this project, I and @fxwiegand have forked this and the minify-html projects to keep the project maintained and keep improving it under a new name and organization.

So with that we would suggest making this PR at our repository here:

https://github.com/web-optim/jsmin/pulls

Have a great day!! 🙂

@RuairidhWilliamson
Copy link
Copy Markdown
Author

Thanks for forking this @neon-mmd

I am using oxc instead of this but have retargeted this PR onto your fork if you want to merge it web-optim/jsmin#2

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.

Panic: "called Option::unwrap() on a None value"

5 participants