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

Refactor ctx #1177

Closed
Tracked by #108
hacdias opened this issue Oct 7, 2019 · 8 comments · Fixed by #2378
Closed
Tracked by #108

Refactor ctx #1177

hacdias opened this issue Oct 7, 2019 · 8 comments · Fixed by #2378
Assignees
Labels
effort/hours Estimated to take one or several hours exp/intermediate Prior experience is likely helpful kind/architecture Core architecture of project kind/enhancement A net-new feature or improvement to an existing feature kind/maintenance Work required to avoid breaking changes or harm to project's status quo need/analysis Needs further analysis before proceeding need/maintainers-input Needs input from the current maintainer(s) P1 High: Likely tackled by core team if no one steps up starmaps status/in-progress In progress topic/infra Infrastructure topic/macos MacOS specific topic/perf Performance

Comments

@hacdias
Copy link
Member

hacdias commented Oct 7, 2019

There is just one codebase that is run by one instance of the program at the time. Is it worth having a variable called ctx (context)? That'd certainly be more useful in HTTP (or any other protocol) handlers where each request is different.

While working on #1175 I found it a bit annoying to have to pass ctx around because I needed one single variable.

The idea is to remove ctx where possible and simply export the functions. Also, refactor the code so we don't have named and default exports on the same file as we do right now. That has been the cause for some bugs up until now.

@hacdias hacdias added the kind/maintenance Work required to avoid breaking changes or harm to project's status quo label Apr 22, 2020
@hacdias
Copy link
Member Author

hacdias commented Apr 22, 2020

@lidel I was actually thinking about this now and searched to see if there was an issue about it. And guess what: there is. I think the code needs improvements here and there and there are certainly a lot of things that can be improved.

On my new PRs, I've been avoiding adding things to ctx as it is not really worth it and it just adds trouble.

@hacdias
Copy link
Member Author

hacdias commented Apr 22, 2020

At the same time, we need to come up with a code layout that also avoid circular dependencies.

@lidel lidel added need/maintainers-input Needs input from the current maintainer(s) need/analysis Needs further analysis before proceeding labels Apr 28, 2020
@SgtPooki SgtPooki self-assigned this Apr 28, 2022
@SgtPooki
Copy link
Member

SgtPooki commented Apr 28, 2022

I need to handle this in order to handle #1996

@SgtPooki SgtPooki added P1 High: Likely tackled by core team if no one steps up effort/hours Estimated to take one or several hours exp/beginner Can be confidently tackled by newcomers good first issue Good issue for new contributors kind/architecture Core architecture of project kind/enhancement A net-new feature or improvement to an existing feature status/in-progress In progress topic/infra Infrastructure topic/perf Performance topic/macos MacOS specific labels Apr 28, 2022
@SgtPooki
Copy link
Member

SgtPooki commented Apr 28, 2022

@hacdias im going to be adding typings via jsdoc to ctx, and making it import/require-able

SgtPooki added a commit that referenced this issue Apr 28, 2022
@SgtPooki SgtPooki linked a pull request Apr 28, 2022 that will close this issue
@SgtPooki
Copy link
Member

@hacdias In order to understand what additional improvements should be made: What issues have you seen or ran into with ctx as it previously existed?

Does the PR I just pushed out (#2114) resolve those issues? What issues doesn't it resolve?

@SgtPooki SgtPooki added exp/intermediate Prior experience is likely helpful and removed exp/beginner Can be confidently tackled by newcomers labels May 19, 2022
@tinytb
Copy link

tinytb commented Jan 18, 2023

  • ctx is an object that tries to implement an app context in ipfs-desktop. By refactoring it, we can speed up ipfs-desktop startup time.
  • ctx being difficult to share & maintain has slowed development and caused "gotchas" for new features. feat: app lazy-loads app-context #2378 fixes this.

@AliakbarETH
Copy link

Is this issue resolved or still pending?

@SgtPooki
Copy link
Member

Still pending. I have a PR WIP where I cleaned up quite a bit, but I haven't returned to it for a while.

@SgtPooki SgtPooki removed the good first issue Good issue for new contributors label Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/hours Estimated to take one or several hours exp/intermediate Prior experience is likely helpful kind/architecture Core architecture of project kind/enhancement A net-new feature or improvement to an existing feature kind/maintenance Work required to avoid breaking changes or harm to project's status quo need/analysis Needs further analysis before proceeding need/maintainers-input Needs input from the current maintainer(s) P1 High: Likely tackled by core team if no one steps up starmaps status/in-progress In progress topic/infra Infrastructure topic/macos MacOS specific topic/perf Performance
Projects
No open projects
Status: Done
5 participants