-
-
Notifications
You must be signed in to change notification settings - Fork 26
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
[BREAKING CHANGE] Replace constructor function to class declaration #30
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future please provide:
- a description and reasoning
- how to test
We should also include something like this in a PR template across the repos where there is time.
For example:
It changes all the modules to use ES6 class
-syntax instead of mimicking classes using prototypal inheritance. The reason we can do this without babel is because we're targeting >[email protected]
in ./package.json
.
How to test:
git clone -b replace-class https://github.com/segayuu/warehouse.git
cd warehouse
npm install
npm test
I found a problem integrating it with https://github.com/hexojs/hexo. git clone -b replace-class https://github.com/segayuu/warehouse.git
cd warehouse
npm install
cd ..
git clone https://github.com/hexojs/hexo.git
cd hexo
npm install
npm rm warehouse
npm install ../warehouse
npm test The result is this error:
@segayuu Maybe you can advise me here. Unless I did something wrong we may need to adjust other modules for compatibility. |
I appreciate @tcrowe 's review! This error message is not a bug. Either way, the |
I think, following procedure is better way for migrate warehouse repository to class declaration.
I think hexo's core repositories should to be migrate ES6 step by step. |
Ten days have passed since the last comment, and there are reactions with pictograms, so I will change the README.md and request a review again. |
- Writing a text page assuming that major version upgrade was done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it and it works well. Thank you, @segayuu @yoshinorin 👍
No description provided.