Skip to content
This repository has been archived by the owner on Nov 17, 2020. It is now read-only.

Use Uglifier Harmony mode #62

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use Uglifier Harmony mode #62

wants to merge 1 commit into from

Conversation

michaelbaudino
Copy link
Member

This commit overrides the default Rails configuration to use the Harmony
mode of Uglifier, which support ES6 syntax, as mentioned in their README
file:
https://github.com/lautis/uglifier#es6--es2015--harmony-mode

@michaelbaudino michaelbaudino self-assigned this Dec 11, 2018
@michaelbaudino michaelbaudino requested a review from a user December 11, 2018 17:36
@michaelbaudino michaelbaudino temporarily deployed to toolbox-staging-pr-62 December 11, 2018 17:36 Inactive
@michaelbaudino michaelbaudino temporarily deployed to toolbox-staging-pr-62 December 11, 2018 17:46 Inactive
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Let's remove the unneeded magic comment and remove the comment, then merge it.

@@ -0,0 +1,7 @@
# frozen_string_literal: true
Copy link

Choose a reason for hiding this comment

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

There are no string literals in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know, but our Rubocop ruleset makes the magic comment mandatory 🤷‍♂️

Copy link

Choose a reason for hiding this comment

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

OK, maybe we could reconsider whilelisting rules vs. blacklisting? ;-)

config/initializers/uglifier.rb Outdated Show resolved Hide resolved
config/initializers/uglifier.rb Show resolved Hide resolved
This commit overrides the default Rails configuration to use the Harmony
mode of Uglifier, which support ES6 syntax, as mentioned in their README
file:
https://github.com/lautis/uglifier#es6--es2015--harmony-mode
@ghost
Copy link

ghost commented Dec 12, 2018

I was going to merge, but I'll avoid since the build is "broken".
However, I think it's only because of some default rubocop rules :-(

@michaelbaudino
Copy link
Member Author

Hrm, it's interesting 🤔

The current offense is actually due to our codestyle disabling Metrics/LineLength, thus the Style/IfUnlessModifier has no length limit to try to rewrite if condition with single-line body (if something; do_this; end) as a modifier (do_this if something).

It expects us to rewrite this if statement like that:

Rails.application.config.assets.js_compressor = Uglifier.new(harmony: true) if Rails.application.config.assets.js_compressor == :uglifier

Which is actually obviously too long 🤷‍♂️

I see 3 options here:

  1. Disable Style/IfUnlessModifier in our codestyle

  2. Enable Metrics/LineLength in our codestyle (but iirc we agreed it could be a pain and was an overall good practice anyway, not Ruby-specific)

  3. Use the following workaround here:

    if Rails.application.config.assets.js_compressor == :uglifier
      Rails.application.config.assets.js_compressor =
        Uglifier.new(harmony: true)
    end

    ... because Style/IfUnlessModifier doesn't run since the body is now 2 lines long 🙈

What do you think?

@ghost
Copy link

ghost commented Dec 13, 2018

I agree that Rubocop suggestion isn't acceptable here :-(

  1. I'd prefer this option, given the context.
  2. I'm not against enabling it, but I agree it "shouldn't" be necessary.
    However, I'm not sure the "pain" was about line length, IIRC the annoying
    ones were block lengths or other similar metrics.
  3. I dislike it: "problems/bugs" of our linter shouldn't have an impact on how
    we write code.

So personally, I'd go with first option.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant