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

Uglifyjs fix #1063

Closed
wants to merge 2 commits into from
Closed

Uglifyjs fix #1063

wants to merge 2 commits into from

Conversation

ocboogie
Copy link

@ocboogie ocboogie commented Apr 13, 2018

Note: I'm not quite sure if Preact is the problem.

I came across a problem using react-slick and preact. I got a Uncaught TypeError: Cannot read property 'className' of undefined, but the odd thing was I only got this error in production mode. So I looked into it and it turns out preact-compat wasn't mutating the VNode. And the reason for that was Uglifyjs was not exposing VNode anymore. Since VNode is an empty class and only used once, in the h function, Uglifyjs makes it inline, like let p = new function() {}(); so preact-compat can't access it.

You can see this issue in action here.

So my fix(hack) for this issue was setting a property on VNode after it was declared, this way Uglifyjs doesn't make it inline. They're very well might be a more elegant way to fix this.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 50a6f41 on ocboogie:uglifyjs-fix into 3e69287 on developit:master.

@yaodingyd
Copy link
Collaborator

Did you try using preact-compat? I think this line https://github.com/developit/preact-compat/blob/master/src/index.js#L41 covers your issue.

@ocboogie
Copy link
Author

ocboogie commented Apr 13, 2018

@yaodingyd Unfortunately that's not the case. Since the VNode class is inline in the h function, it creates a new class each time h is ran. So when preact-compat is mutating the constructor, it only mutates that Instance.

I added an example in my test environment here. This will log true.

@yaodingyd
Copy link
Collaborator

@ocboogie Just realize I didn't fully read your first comment. My bad! Will investigate more.

@ocboogie
Copy link
Author

ocboogie commented Apr 14, 2018

Just so you know the compression option that's causing problems is the unused option. You can see it here. And you can disable it in the Webpack config here.

@kaisermann
Copy link

What version of uglifyjs are you using? If it's >= 3, turn off the compress.reduce_funcs options and see if it works.

@yaodingyd
Copy link
Collaborator

It looks like setting either unused or reduce_funcs to false both works.

@developit
Copy link
Member

I would prefer to solve this by documenting the issue - adding weight and cost to Preact to fix an Uglify bug isn't something we can do. Anyone want to take a swing at adding it to the docs?

@developit
Copy link
Member

Anyone know if source position matters to Uglify here? Also is there an upstream bug we can track to know when Uglify fixes this?

@developit
Copy link
Member

Hi all! This was fixed in Preact 8.3.0 thanks to @wardpeet's clever trick (#1153).

@developit developit closed this Aug 17, 2018
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.

None yet

6 participants