Skip to content

chore(eslint): added eslint-node-plugin#3652

Merged
TheLarkInn merged 2 commits into
masterfrom
feature/eslint_node_plugin_with_refactor
Dec 30, 2016
Merged

chore(eslint): added eslint-node-plugin#3652
TheLarkInn merged 2 commits into
masterfrom
feature/eslint_node_plugin_with_refactor

Conversation

@TheLarkInn

Copy link
Copy Markdown
Member

What kind of change does this PR introduce?
Refactor, ESLINT ❤️

Did you add tests for your changes?
None needed!

If relevant, link to documentation update:
N/A

Summary
Because we have to support node >=4.7 we need to have a easy way to ensure that all linting and node api features are correctly in place.

This PR adds eslint-node-plugin which allows us to essentially have our engine property set, and then get intelligent and appropriate linting settings with the exact "ecmaFeatures" that we are looking for. Aside from that I fixed any lint warnings in master. Now folks will be forced to add "use strict"

Does this PR introduce a breaking change?
No.

Other information

sourceMap.sourceRoot = options.sourceRoot || "";
sourceMap.file = module.id + ".js";
var footer = self.sourceMapComment.replace(/\[url\]/g, "data:application/json;charset=utf-8;base64," + new Buffer(JSON.stringify(sourceMap)).toString("base64"));
var footer = self.sourceMapComment.replace(/\[url\]/g, "data:application/json;charset=utf-8;base64," + Buffer.from(JSON.stringify(sourceMap)).toString("base64"));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

new Buffer() has been deprecated since node 6. So Using Buffer.from()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Buffer.from is not existed on node@v4

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It does infact exist on node 4.7>.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup this got reverted!

Comment thread bin/convert-argv.js
options.plugins.push(new ProvidePlugin(name, value));
});

ifBooleanArg("labeled-modules", function() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why to remove this? Should the removal be a separate PR?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So this is because The file doesn't exist. This was also a lint error.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok.

Comment thread lib/Compiler.js
@@ -321,7 +321,7 @@ Compiler.prototype.emitAssets = function(compilation, callback) {
}
var content = source.source();
if(!Buffer.isBuffer(content))

@bebraw bebraw Dec 30, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would force braces with ifs (separate PR) as the form without can hide errors easily.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes thats a good idea

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll do that in a separate PR. Otherwise looks good?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yup.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe "curly": ["error", "multi-line", "consistent"], to still allow for short ifs that cannot visually be confused with a block?

@TheLarkInn

Copy link
Copy Markdown
Member Author

Rerunning travis to make sure the PR hook was just a flake run. Otherwise maybe related to yarn PR merge.

@TheLarkInn TheLarkInn removed the request for review from SpaceK33z December 30, 2016 20:05
@TheLarkInn TheLarkInn merged commit 7327ee6 into master Dec 30, 2016
@TheLarkInn TheLarkInn deleted the feature/eslint_node_plugin_with_refactor branch December 30, 2016 23:13
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.

4 participants