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

fix (typings): Upgrade to the latest version of 'egg-cookie' to fetch 'encrypted' in intellisense #2958

Merged
merged 1 commit into from Sep 10, 2018

Conversation

ghost
Copy link

@ghost ghost commented Sep 1, 2018

Ref:
1)eggjs/egg-cookies#12
2)eggjs/egg-cookies#11
3)#2949
4)eggjs/egg-cookies#15

In short:
The Egg's cookie isn't inheriting from Koa's Context's cookie, we should rewrite it by adding 'encrypted' as the prop.

Self-Link:
#2958

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

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 3, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2958   +/-   ##
=======================================
  Coverage   99.75%   99.75%           
=======================================
  Files          29       29           
  Lines         826      826           
=======================================
  Hits          824      824           
  Misses          2        2

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 a2df5ad...f697fb6. Read the comment docs.

@ghost ghost changed the title lib: Override the cookies property in Egg's Context fix: Upgrade to the latest version of 'egg-cookie' to fetch 'encrypted' in intellisense Sep 6, 2018
@ghost ghost changed the title fix: Upgrade to the latest version of 'egg-cookie' to fetch 'encrypted' in intellisense fix (typings): Upgrade to the latest version of 'egg-cookie' to fetch 'encrypted' in intellisense Sep 6, 2018
@ghost
Copy link
Author

ghost commented Sep 6, 2018

@whxaxes:Notice that I've referred the latest version of egg-cookies's index.d.ts. And this has exposed 'ctx', which isn't a must in 'egg' because we don't use something like 'this.ctx.cookies.ctx.……'. So I've created another interface as a wrapper to disclose the 'ctx' and expose ONLY get/set methods. Correct me if any suggestions :)

index.d.ts Outdated Show resolved Hide resolved
@atian25
Copy link
Member

atian25 commented Sep 7, 2018

need rebase

'encrypted' in intellisense

Ref:
1)eggjs/egg-cookies#12
2)eggjs/egg-cookies#11
3)#2949

In short:
The Egg's cookie isn't inheriting from Koa's Context's cookie, we should
rewrite it by adding 'encrypted' as the prop.

Self-Link:#2958
@ghost
Copy link
Author

ghost commented Sep 7, 2018

@atian25 :OK.

@atian25
Copy link
Member

atian25 commented Sep 7, 2018

cc @dead-horse plz raise a release PR

@atian25 atian25 merged commit 815c278 into eggjs:master Sep 10, 2018
popomore pushed a commit that referenced this pull request Sep 10, 2018
fix (typings): Upgrade to the latest version of 'egg-cookie' to fetch (#2958)
@ghost ghost deleted the FixCookiesPropInTs branch September 10, 2018 02:31
@jsl9208
Copy link

jsl9208 commented Dec 19, 2018

Context 接口使用 RemoveSpecProp 这个方式扩展后 (index.d.ts L685),tsserver 对 ctx 的补全就不包含 koa.Context 里面的字段了,改回之前的写法就没问题。请问可能是什么原因?

@whxaxes
Copy link
Member

whxaxes commented Dec 19, 2018

Context 接口使用 RemoveSpecProp 这个方式扩展后 (index.d.ts L685),tsserver 对 ctx 的补全就不包含 koa.Context 里面的字段了,改回之前的写法就没问题。请问可能是什么原因?

@jsl9208 之前是没问题的,现在我刚试了一下好像真的有这种情况,我查一下原因,之后发个 PR 修复

@whxaxes
Copy link
Member

whxaxes commented Dec 19, 2018

@jsl9208

找到原因了。。。最近 koa 的声明发了个版本,给 Context 加了 [key: string]: any 的声明,导致 keyof Context 就没法列出 koa.Context 里的所有属性了 ...

我今天想办法修复一下

@whxaxes whxaxes mentioned this pull request Dec 19, 2018
4 tasks
@whxaxes
Copy link
Member

whxaxes commented Dec 19, 2018

@jsl9208 PR 已提: #3329

@jsl9208
Copy link

jsl9208 commented Dec 19, 2018

@whxaxes 好的,多谢!

@atian25
Copy link
Member

atian25 commented Dec 20, 2018

land 2.14.2

hey, please retry after reinstall dependencies and please never lock it.

$ # reinstall deps and never lock it.
$ rm -rf node_modules yarn.lock package-lock.json
$ npm i --no-package-lock

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.

None yet

3 participants