Skip to content
This repository was archived by the owner on Oct 29, 2025. It is now read-only.

Dev multi app#82

Merged
ilaipx merged 3 commits into
devfrom
dev-multi-app
Mar 13, 2019
Merged

Dev multi app#82
ilaipx merged 3 commits into
devfrom
dev-multi-app

Conversation

@ilaipx

@ilaipx ilaipx commented Mar 11, 2019

Copy link
Copy Markdown
Contributor

No description provided.

ilaipx added 2 commits March 11, 2019 19:41
…through functions.

[CHANGE] pxlogger is now externalized since it's needed for instantiation.
[TODO] request is still a singleton.
[CHANGE] pxClient now requires config in init() method.
[CHANGE] setting the agent for request has a new method.
[REFACTOR] PxConfig to have less redundant code.
[CHANGE] moved extractIP from pxcontext to utils.
[ADD] changelog notes.
@yaronschwimmer

Copy link
Copy Markdown

Why do you need to inject the logger everywhere now? what was the problem with the way it was before?

@ilaipx

ilaipx commented Mar 12, 2019

Copy link
Copy Markdown
Contributor Author

It has a state (debug and app id) which differs between apps, and it was global (singleton) before.

@yaronschwimmer

Copy link
Copy Markdown

it makes no sense to pass the logger around like that. find a better way. maybe put it on the context?

@ilaipx

ilaipx commented Mar 12, 2019

Copy link
Copy Markdown
Contributor Author

Yes, it's possible. There are many things that are not as I would like there. Tried to make minimal changes. If we put it in context we will still have many places to inject (in the small functions that don't have the context) so it will only solve a part of the problem.

[REFACTOR] injected logger into config to use it from config
[REFACTOR] pxCtx -> ctx
[FIX] creating the default and internal configs to remain stateless.
@ilaipx ilaipx merged commit 91e776a into dev Mar 13, 2019
@ilaipx ilaipx deleted the dev-multi-app branch March 13, 2019 09:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants