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

feat: usage of ES6 class syntax #253

Merged
merged 4 commits into from
Sep 14, 2021
Merged

Conversation

theoludwig
Copy link
Contributor

@theoludwig theoludwig commented Aug 18, 2021

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix
[x] New feature
[ ] Other, please explain:

What changes did you make? (Give an overview)

Before:

const { linter } = require('standard-engine')

After:

const { Linter } = require('standard-engine')

Which issue (if any) does this pull request address?

Fixes #250

Is there anything you'd like reviewers to focus on?

BREAKING CHANGES introduced by this PR:

  • Export is now called Linter, not linter
  • Calling Linter() instead of new Linter() doesn't work anymore.

Copy link
Member

@voxpelli voxpelli left a comment

Choose a reason for hiding this comment

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

Overall 👍 Some minor thoughts, but nothing major.

I like the size of this PR, will be possible to wrap up and merge quite quickly.

Breaking changes introduced by this PR:

  • Export is now called Linter, not linter
  • It may be that calling Linter() instead of new Linter() doesn't work anymore.

README.md Show resolved Hide resolved
Comment on lines +153 to +152
module.exports.cli = require('./bin/cmd')
module.exports.Linter = Linter
Copy link
Member

Choose a reason for hiding this comment

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

This casing change isn't strictly caused by ES6, but I do like it and it should be done eventually.

I would prefer this export:

Suggested change
module.exports.cli = require('./bin/cmd')
module.exports.Linter = Linter
module.exports = {
cli: require('./bin/cmd'),
Linter
};

Copy link
Contributor Author

@theoludwig theoludwig Aug 18, 2021

Choose a reason for hiding this comment

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

Changed the export but without the semicolon, that's why I didn't commit your suggestion. 😄

EDIT: It is not working with the StandardCliOptions type inside bin/cmd.js.

Error extracted from the test failing in the GitHub Actions:
Error: bin/cmd.js(8,29): error TS2694: Namespace '"/home/runner/work/standard-engine/standard-engine/index".export=' has no exported member 'Linter'.

So it's probably better to do the exports like that:

module.exports.cli = require('./bin/cmd')
module.exports.Linter = Linter

index.js Outdated Show resolved Hide resolved
@theoludwig theoludwig closed this Aug 18, 2021
@theoludwig theoludwig force-pushed the feat/usage-of-es6-class-syntax branch from 7a8ab97 to 84f2a37 Compare August 18, 2021 17:12
@theoludwig theoludwig reopened this Aug 18, 2021
@theoludwig
Copy link
Contributor Author

Could you review again my PRs (this one and #252) when you have some time? @voxpelli
Thank you very much for your help! 😄

Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

Neat 👍

Copy link
Member

@voxpelli voxpelli left a comment

Choose a reason for hiding this comment

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

Excellent! Very good improvement

bin/cmd.js Outdated
Comment on lines 26 to 27
const { Linter } = require('../')
const standard = rawOpts.linter || new Linter(opts)
Copy link
Member

Choose a reason for hiding this comment

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

The intention with doing require('../') here rather than in the top of the file must have been to avoid loading it if another linter had been provided, but then the code would have to be:

Suggested change
const { Linter } = require('../')
const standard = rawOpts.linter || new Linter(opts)
const standard = rawOpts.linter || new ( require('../').Linter)(opts)

Just an observation 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow, if I put const { Linter } = require('../') at the top of the file, the tests are failing :
TypeError: Linter is not a constructor.
Do you have an idea, why?
Maybe you can test it yourself locally (I'm using Node.js v16.9.1).

But, yes if I take your recommendation const standard = rawOpts.linter || new (require('../').Linter)(opts), it is working, I'll change it then. 👍

@@ -210,3 +208,5 @@ Flags (advanced):
}
}
}

module.exports = cli
Copy link
Member

Choose a reason for hiding this comment

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

I like the move to the bottom and the lowercasing of it!

@theoludwig theoludwig merged commit b92f2da into master Sep 14, 2021
@theoludwig theoludwig deleted the feat/usage-of-es6-class-syntax branch September 14, 2021 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Move to ES6 class syntax instead of prototypes
3 participants