Skip to content

tools: remove minifying logic - #6636

Closed
thefourtheye wants to merge 1 commit into
nodejs:masterfrom
thefourtheye:fix-jsmin-import
Closed

tools: remove minifying logic#6636
thefourtheye wants to merge 1 commit into
nodejs:masterfrom
thefourtheye:fix-jsmin-import

Conversation

@thefourtheye

@thefourtheyethefourtheye commented May 8, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • the commit message follows commit guidelines
Affected core subsystem(s)

tools

Description of change

jsmin module is imported to use JavaScriptMinifier class, but the
class should be referenced with the module name. Otherwise, it will
throw a NameError at runtime.

As we don't use minifier at all, this patch removes the entire minifying logic.

cc @bnoordhuis

CI Run: https://ci.nodejs.org/job/node-test-pull-request/2538/

@thefourtheyethefourtheye added the tools Issues and PRs related to the tools directory. label May 8, 2016
@bnoordhuis

Copy link
Copy Markdown
Member

We don't minify so I'd simply remove that code. You can probably reduce the line count by quite a bit if you remove all minification-related code.

@thefourtheye

Copy link
Copy Markdown
ContributorAuthor

@bnoordhuis The minifier actually has a bug, I was thinking about fixing it. Are we never going to use minifier?

@thefourtheye

Copy link
Copy Markdown
ContributorAuthor

Okay, removed the entire minifying logic now.

@bnoordhuis

bnoordhuis commented May 8, 2016

Copy link
Copy Markdown
Member

LGTM

EDIT: You'll need to update the commit log, though.

@thefourtheyethefourtheye changed the title tools: use JavaScriptMinifier with module nametools: remove minifying logicMay 8, 2016
As the minifier logic is not used at all, this patch removes the code
necessary for it.
@thefourtheye

Copy link
Copy Markdown
ContributorAuthor

@bnoordhuis Thanks :-) Updated the commit message and log.

@JacksonTian

Copy link
Copy Markdown
Contributor

LGTM

@bnoordhuis

Copy link
Copy Markdown
Member

Commit log LGTM.

@thefourtheye

Copy link
Copy Markdown
ContributorAuthor

Landed in 0e2b250

@thefourtheye
thefourtheye deleted the fix-jsmin-import branch May 12, 2016 14:59
thefourtheye added a commit that referenced this pull request May 12, 2016
As the minifier logic is not used at all, this patch removes the code
necessary for it.
PR-URL: #6636
Reviewed-By: Jackson Tian <shvyo1987@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
evanlucas pushed a commit that referenced this pull request May 17, 2016
As the minifier logic is not used at all, this patch removes the code
necessary for it.
PR-URL: #6636
Reviewed-By: Jackson Tian <shvyo1987@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins

Copy link
Copy Markdown
Contributor

@thefourtheye lts?

@thefourtheye

Copy link
Copy Markdown
ContributorAuthor

@thealphanerd Ya, we can LTS this.

MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
As the minifier logic is not used at all, this patch removes the code
necessary for it.
PR-URL: #6636
Reviewed-By: Jackson Tian <shvyo1987@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorinsMylesBorins mentioned this pull request Jun 24, 2016
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
As the minifier logic is not used at all, this patch removes the code
necessary for it.
PR-URL: #6636
Reviewed-By: Jackson Tian <shvyo1987@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
As the minifier logic is not used at all, this patch removes the code
necessary for it.
PR-URL: #6636
Reviewed-By: Jackson Tian <shvyo1987@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@thefourtheye@bnoordhuis@JacksonTian@MylesBorins