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

Chore (gitignore, file_loader.test.js): Update files #195

Merged
merged 1 commit into from Jan 28, 2019
Merged

Chore (gitignore, file_loader.test.js): Update files #195

merged 1 commit into from Jan 28, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jan 5, 2019

1) For 'gitignore':
Add the 'package-lock.json' to ignore it.

2) For 'file_loader.test.js':
Uncomment one unit test to make full unit test passed.


  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ghost ghost requested review from atian25, dead-horse and killagu January 5, 2019 03:02
@ghost ghost changed the title Chore:Add comments and igore 'package-lock.json' Refactor: Add/remove some comments and move validation checks Jan 5, 2019
@codecov
Copy link

codecov bot commented Jan 5, 2019

Codecov Report

Merging #195 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #195   +/-   ##
=======================================
  Coverage   99.91%   99.91%           
=======================================
  Files          19       19           
  Lines        1215     1215           
  Branches      212      212           
=======================================
  Hits         1214     1214           
  Misses          1        1
Impacted Files Coverage Δ
lib/egg.js 100% <ø> (ø) ⬆️
lib/lifecycle.js 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 b958870...fc3f352. Read the comment docs.

lib/egg.js Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Jan 5, 2019

I'll try to fix this pending in the unit test, this happened before this submit.

image

After my fixture:
image

@ghost ghost requested a review from popomore January 5, 2019 04:25
lib/egg.js Outdated Show resolved Hide resolved
@ghost ghost changed the title Refactor: Add/remove some comments and move validation checks Chore (gitignore, file_loader.test.js): Update files Jan 6, 2019
@ghost
Copy link
Author

ghost commented Jan 6, 2019

@popomore:We must add some deprecated messages to notify users about some NOT-RECOMMANDED methods but keep them still in alive.

lib/lifecycle.js Outdated
@@ -106,10 +106,12 @@ class Lifecycle extends EventEmitter {
}

registerBeforeStart(scope) {
deprecate('beforeStart is deprecated, please use either didReady or willReady instead.');
Copy link
Member

Choose a reason for hiding this comment

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

The deprecate message isn't friendly, I'll create a document for the migration, point to the document later.

Copy link
Author

Choose a reason for hiding this comment

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

Who will see the doc? And you should know that when a method isn't used any more, we should put it as a deprecated.....

Copy link
Author

Choose a reason for hiding this comment

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

In my mind, your doc should be synced with the deprecated methods, this should be right and IMO, I should add this.

Copy link
Member

Choose a reason for hiding this comment

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

死马的意思是depd没问题,需要写一份如何升级的文档,并将文档链接写到depd message 里面去

Copy link
Author

Choose a reason for hiding this comment

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

@fengmk2 :There're some warnings telling us that we shouldn't use some methods, take 'beforeClose' as an example:
image

Copy link
Author

@ghost ghost Jan 7, 2019

Choose a reason for hiding this comment

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

就是引用:https://eggjs.org/en/advanced/loader.html#beforestart。 如果可以我在 deprecate 中引用这个连接。

Copy link
Member

Choose a reason for hiding this comment

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

这个不是 migration 的文档,migration 需要说明老的代码如何迁移到新的写法才是的。

Copy link
Author

Choose a reason for hiding this comment

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

@fengmk2 :我知道这个不是migration,不过这个文档已经很清楚了,确实还需要一份Migration文档?

Copy link
Author

Choose a reason for hiding this comment

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

我建议就在原有文档基础上进行一些补充说明——就像你们Egg从1.X到2.X一样,在源文档上做更新。

Copy link
Member

Choose a reason for hiding this comment

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

对,只要有这份升级说明即可,是否独立写都没问题。

@ghost ghost mentioned this pull request Jan 11, 2019
4 tasks
ghost pushed a commit to eggjs/egg that referenced this pull request Jan 22, 2019
1. This will fix the issue eggjs/core#195 and gives a good answer to #3362. We should notify users how to upgrade their lifecyle events quickly.

2. Fix some '##title', we should directly use 'title:...' instead.

3. Due to the conflict of typescript of nodejs in the global/local, we've forcely defined the type path.
popomore pushed a commit to eggjs/egg that referenced this pull request Jan 22, 2019
doc:Add new loaderUpdate.md (#3395)

1. This will fix the issue eggjs/core#195 and gives a good answer to #3362. We should notify users how to upgrade their lifecyle events quickly.

2. Fix some '##title', we should directly use 'title:...' instead.

3. Due to the conflict of typescript of nodejs in the global/local, we've forcely defined the type path.
lib/lifecycle.js Outdated Show resolved Hide resolved
lib/lifecycle.js Outdated Show resolved Hide resolved
1) For 'gitignore':
Add the 'package-lock.json' to ignore it.

2) For 'file_loader.test.js':
Uncomment one unit test to make full unit test passed.
@dead-horse dead-horse merged commit 29118e5 into eggjs:master Jan 28, 2019
@ghost ghost deleted the AddNewComments branch January 28, 2019 11:37
hyj1991 pushed a commit to hyj1991/egg-core that referenced this pull request Sep 16, 2021
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