[don't merge] Closure compiler

[maciej-ka]

#1360

Waits for:

1. Merging Flat bundles for each possible entry #1362 which is the base for this Closure branch
2. Fix of Node process runs out of memory google/closure-compiler-js#23; My machine couldn't pass the error JavaScript heap out of memory and I never saw the result of this uglifier replace.

[mxstbr]

My machine couldn't pass the error JavaScript heap out of memory and I never saw the result of this uglifier replace.

Well that sucks, thanks for trying! Maybe @gaearon also ran into this when switching to Closure Compiler in React?

[gaearon]

No, we didn’t run into these issues. Note we’re using a slightly older fork (see our Yarn lockfile). But the issue seems older than that (we had to fork for a different reason).

Have you tried using the same exact options that we use?

[maciej-ka]

Same result with https://github.com/facebook/react/blob/master/scripts/rollup/build.js#L62 and permutations of these options (including assumeFunctionWrapper and renaming flags).

My machine can build current master of React. I used version "1.0.6" of rollup plugin both in this PR and React master.

[gaearon]

Any chance you're accidentally processing unnecessary code? e.g. maybe it imports React itself by mistake.

[maciej-ka]

Build is passing when Stylis is not used.

Which is when import, stylis const and return statement are removed from
https://github.com/styled-components/styled-components/blob/master/src/utils/stringifyRules.js

[mxstbr]

Huh, interesting! /cc @thysultan any ideas what's up with that?

[maciej-ka]

Looking at Stylis source, problem is present when any of char, format or context variables are changed within part labeled as:

// aggressive isolation mode, divide each individual selector
// including selectors in :not function but excluding selectors in :global function of the code that ignores comments.

The lines that, if commented out, will allow Closure compiler to pass, are:
https://github.com/thysultan/stylis.js/blob/master/stylis.js#L760
https://github.com/thysultan/stylis.js/blob/master/stylis.js#L764
https://github.com/thysultan/stylis.js/blob/master/stylis.js#L767
https://github.com/thysultan/stylis.js/blob/master/stylis.js#L772
https://github.com/thysultan/stylis.js/blob/master/stylis.js#L776-L779
https://github.com/thysultan/stylis.js/blob/master/stylis.js#L804-L805

[thysultan]

Note sure, could be a bug with the JavaScript version of closure compiler since stylis works with both the basic and advanced mode of the Java version.

@maciej-ka What do you mean by Build is passing when Stylis is not used. what error does it throw with it?

[maciej-ka]

@thysultan, when building Styled Components with Stylis, it fails with error: JavaScript heap out of memory, just like google/closure-compiler-js#23

This error is gone when Stylis import or few lines of Stylis code are commented out. They are listed in comment above.

Trouble is happening in selector isolation mode. The irony is that this part of Stylis source is never used by Styled Components, because it's disabled by a setting cascade: true.

(If this can be any help: with cascade: true selectors will be like .user p a, with cascade: false they will be like p.user a.user)

[maciej-ka]

Closure compiler build will pass also if just this one line is commented out https://github.com/thysultan/stylis.js/blob/master/stylis.js#L469

[maciej-ka]

Solid solving of this Closure Compiler issue is beyond my powers. But here is a little change in Stylis that has no impact on its logic and for a mysterious reason, it makes the build pass. thysultan/stylis#90 @thysultan

[gaearon]

Can you file a new issue against GCC with a reproducing minimal case?

[maciej-ka]

Minimal failing example is my dream here for some time. Maybe it will be possible to distill it from Stylis workaround. Another dream is to debug closure compiler but I have no idea how to even start that.

[wardpeet]

@maciej-ka thanks for taking this one, wanted to start on it last week but saw you already took this one. 🎉

[maciej-ka]

I don't plan to spend more time on extracting minimal repro from seven hundred parser Stylis function. I tried. My idea was to cut the code in binary search way, but it seems parts need each other to break closure compiler.

I'm sending my ❤️ to anyone who would like to give this ticket some time.

[SavePointSam]

Just ran into this myself. Pretty bummed I have to choose between styled-components or GCC. :(

[maciej-ka]

@thysultan any chance to merge this temporary fix? I understand it's from your perspective a strange and unnecessary change in code, but it:

• solves the problem,
• has a minimal impact (change is a move of one line variable declaration into broader scope),
• has comes with documentation, describing why this change and linking related ticket.

It's not uncommon to see such walkarounds in open source projects
(yeah, I'm trying hard to sell this)

[quantizor]

@maciej-ka any news on this PR? I'm open to landing it but has been inactive for a while.

[quantizor]

Closing due to inactivity and the codebase changing a lot for v4. Feel free to reimplement and open a new PR if there's interest