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

Migrate to Webpack & ES6 modules #325

Merged
merged 8 commits into from
Feb 23, 2019
Merged

Migrate to Webpack & ES6 modules #325

merged 8 commits into from
Feb 23, 2019

Conversation

Neo-Zhixing
Copy link
Contributor

@Neo-Zhixing Neo-Zhixing commented Feb 23, 2019

Fixes: #277

  • Check the commit's or even all commits' message styles matches our requested structure.
  • Check your code additions will not fail unit test.
  • Check your code additions will not fail code linting checks.
    Indentation was intentionally kept the way it originally was to minimize changes. Automatic formatting could be done at some point in the future.

Short description of what this resolves:

Removed Grunt and enabled Webpack
Migrate the code base to use ES6 modules

Proposed changes:

Further refactor the code base and progressively make everything ES6

@Neo-Zhixing
Copy link
Contributor Author

Currently the bundled umd module has a size of more than 600KB. Work is needed to eliminate some of the dependencies from the build.

@codecov
Copy link

codecov bot commented Feb 23, 2019

Codecov Report

Merging #325 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           develop   #325   +/-   ##
======================================
  Coverage      100%   100%           
======================================
  Files            1      1           
  Lines            1      1           
  Branches         0      1    +1     
======================================
  Hits             1      1
Impacted Files Coverage Δ
dist/htmlhint.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a37bc2...1022e4b. Read the comment docs.

@thedaviddias thedaviddias merged commit 85a1c78 into htmlhint:develop Feb 23, 2019
@Neo-Zhixing Neo-Zhixing mentioned this pull request Feb 23, 2019
2 tasks
thedaviddias pushed a commit that referenced this pull request Feb 23, 2019
**Fixes**: #325 

- [x] Check the commit's or even all commits' message styles matches our requested structure.
- [x] Check your code additions will fail neither code linting checks nor unit test.

#### Short description of what this resolves:
In the original pull request some of the test cases were missing. Added --recursive option for mocha to check for all individual rules.
@Arcanemagus
Copy link

This completely broke the Node.js API, with no changelog entry, no release notes on the release, and most importantly no update to the documentation.

It looks like now the HTMLHint being exported is a Class instead of a function, meaning we need to instantiate it.
Before and the documentation:

var HTMLHint  = require("htmlhint").HTMLHint;
var messages = HTMLHint.verify('<ul><li></ul>', {'tag-pair': true});

Now:

var HTMLHintClass  = require("htmlhint").HTMLHint;
var HTMLHint = new HTMLHintClass();
var messages = HTMLHint.verify('<ul><li></ul>', {'tag-pair': true});

However this appears to return an empty array no matter what I feed it, so something else has changed as well.

Breaking changes are okay as long as they are clearly communicated, this is an example of how not to do them.

@thedaviddias
Copy link
Member

Hi @Arcanemagus, thanks to point that out, we were planning to fix these issues after the weekend where students worked on the project during Hackillinois.

We communicated on the discord channel about but be sure another release will be done in the next days to fix that.

It was an opportunity for students to work on open-source projects and yes, it comes with mistakes but we were aware of that. Thanks again.

@Arcanemagus
Copy link

Glad to hear it! I'll check back when AtomLinter/linter-htmlhint#197 updates 😉.

@thedaviddias
Copy link
Member

By the way, they won the prize for "the best contribution to a code review tool" 🙌

@Arcanemagus
Copy link

Reminder that the currently released version of the package is still broken 😉.

@Arcanemagus
Copy link

The currently released version (v0.11.0) is still broken.

@Arcanemagus
Copy link

The current release still hasn't been fixed, or the documentation updated to match whatever the new behavior is. 🤷‍♂

@Arcanemagus
Copy link

For anyone else coming to this, it looks like this is the new way to get a working HTMLHint.verify:

const htmlhintModule = require('htmlhint');
const HTMLHint = new htmlhintModule.HTMLHint();
Object.keys(htmlhintModule.HTMLRules).forEach((rule) => {
  HTMLHint.addRule(htmlhintModule.HTMLRules[rule]);
});

Taken from the code used here to generate what you get at require('htmlhint').default.

thedaviddias pushed a commit that referenced this pull request Feb 1, 2020
**Fixes**: #325 

- [x] Check the commit's or even all commits' message styles matches our requested structure.
- [x] Check your code additions will fail neither code linting checks nor unit test.

#### Short description of what this resolves:
In the original pull request some of the test cases were missing. Added --recursive option for mocha to check for all individual rules.
thedaviddias added a commit that referenced this pull request May 13, 2020
* fix: missing test cases (#327)

**Fixes**: #325 

- [x] Check the commit's or even all commits' message styles matches our requested structure.
- [x] Check your code additions will fail neither code linting checks nor unit test.

#### Short description of what this resolves:
In the original pull request some of the test cases were missing. Added --recursive option for mocha to check for all individual rules.

* feat(htmlhint): add new rule for whitespace in attributes (#310) (#322)

* 0.10.3

* feat(htmlhint): add new rule for whitespace in attributes (#310)

* fix(htmlhint): removed unnecessary code (#310)

* 0.11.0

* Revert "Merge branch 'develop' into attr-whitespace"

This reverts commit 35988c1, reversing
changes made to 981cca7.

* Revert "Revert "Merge branch 'develop' into attr-whitespace""

This reverts commit cfa794d.

* chore(htmlhint): rebuilt package.json

* chore(htmlhint): updated rule to use ES6

* 0.10.3

* 0.11.0

* fix(htmlhint): made change to test cases and index.js

* html.js (#304)

This is a new formatter which logs error messages in html format on console as well as it will generate a report file in html format

**Fixes**: #

🚨 Please review the [guidelines for contributing](CONTRIBUTING.md) and our [code of conduct](../CODE_OF_CONDUCT.md) to this repository. 🚨
**Please complete these steps and check these boxes (by putting an x inside the brackets) before filing your PR:**

- [x] Check the commit's or even all commits' message styles matches our requested structure.
- [x] Check your code additions will fail neither code linting checks nor unit test.

#### Short description of what this resolves:
New formatter

#### Proposed changes:
New formatter

## -

-

👍 Thank you!

* fix(htmlhint): added rule for special characters in tag name (#146) (#331)

**Fixes**: #

🚨 Please review the [guidelines for contributing](CONTRIBUTING.md) and our [code of conduct](../CODE_OF_CONDUCT.md) to this repository. 🚨
**Please complete these steps and check these boxes (by putting an x inside the brackets) before filing your PR:**

- [x] Check the commit's or even all commits' message styles matches our requested structure.
- [x] Check your code additions will fail neither code linting checks nor unit test.

#### Short description of what this resolves:
Added rule to check for special characters in tag name. See (#146)
#### Proposed changes:

## -

-

👍 Thank you!

* fix(htmlhint): added rule for quotes around attribute values (#147) (#333)

* fix(htmlhint): add ability to pass regexp in config (#328)

Based on #238

Add ability to pass regexp in `.htmlhintrc` file for matching attributes value.

Example:
```js
...
"id-class-value": {
    "regId": "^[a-z\\d]+([-_]+[a-z\\d]+)*$",
    "message": "The id and class attribute values must be in lowercase and split by dash or underscore"
},
...
```

Problem was in parsing config file by `JSON.parse`. We can not correct pass `regexp` into json schema. Only `string`. And this `string` after parsing is `string` too.

I just check input data. If it is not regexp then create `new RegExp` instance from input string.

**Fixes**: #

🚨 Please review the [guidelines for contributing](CONTRIBUTING.md) and our [code of conduct](../CODE_OF_CONDUCT.md) to this repository. 🚨
**Please complete these steps and check these boxes (by putting an x inside the brackets) before filing your PR:**

- [ ] Check the commit's or even all commits' message styles matches our requested structure.
- [ ] Check your code additions will fail neither code linting checks nor unit test.

#### Short description of what this resolves:

#### Proposed changes:

## -

-

👍 Thank you!

* feat(htmlhint): added attribute sorting (#309) (#332)

**Fixes**: #

🚨 Please review the [guidelines for contributing](CONTRIBUTING.md) and our [code of conduct](../CODE_OF_CONDUCT.md) to this repository. 🚨
**Please complete these steps and check these boxes (by putting an x inside the brackets) before filing your PR:**

- [ ] Check the commit's or even all commits' message styles matches our requested structure.
- [ ] Check your code additions will fail neither code linting checks nor unit test.

#### Short description of what this resolves:
Added sort feature for tag attributes.
#### Proposed changes:
-
## -

-

👍 Thank you!

* Used Object.keys instead of Object.values (#344)

Used Object.keys instead of Object.values because it is not supported by nodejs before v8.

* Create FUNDING.yml (#350)

* updates URLs in repository and bugs in package.json (#354)

* chore: update commitizen and semantic release (#357)

* feat(htmlhint): add RegExp and regex string (#346)

RegExp and regex string can be used in whitelist now!
related to #228 #183 microsoft/vscode-htmlhint#34

* update links back to htmlhint.com (#359)

Co-authored-by: Zhixing Zhang <account@neoto.xin>
Co-authored-by: Daman Mulye <dmulye2@illinois.edu>
Co-authored-by: nmanupuri <39561219+nmanupuri@users.noreply.github.com>
Co-authored-by: orangewit3 <43012690+orangewit3@users.noreply.github.com>
Co-authored-by: meetDeveloper <32453612+meetDeveloper@users.noreply.github.com>
Co-authored-by: Christian Oliff <christianoliff@yahoo.com>
Co-authored-by: New Future <NewFuture@users.noreply.github.com>
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.

3 participants