codemap removal #1245
codemap removal #1245
Conversation
|
So I just learned something amazing. When whomever is going to review this goes for it, please add |
|
Not sure what that really need for project, i'm more worry about logic of indicators and strategies. |
|
You're not wrong - there are a lot of other pressing issues to be addressed, but I saw this one as a fairly large barrier to entry for developers and a fundamentally poor practice from a performance standpoint. If we make the project easier to develop on, everyone benefits and perhaps we can get more contributors easier. |
|
Crazy, but nice work. There are a ton of changes here, but looking through it, it seems pretty solid to me. I think this will make it easier for people to jump into the code. I think the original intent of the codemap was for some sort of inversion of control pattern, but I'm not a fan of this implementation. Good tip on add |
|
I agree codemap needs to go, but I think more value is added by continuing with the refactor I started in PR #1214. It establishes a pattern that can be used in other functionality in the application, including services, factories, functions, and unit tests. Following that path will reduce a lot of the current barrier to entry. I say wait on this for now. Or @krystophv, perhaps we can merge our efforts somehow. |
|
While I've never been a huge fan of the 'service' pattern, I do think that your work in #1214 is moving in the right direction. If one of these PR's gets merged before the other, it'll mean a significant amount of work updating the other before it'll be ready again. I'm fine if #1214 gets in and then I have to update this one. |
|
I'll re-merge down from unstable and update this PR again. |
|
@krystophv thank you, when you've done that I'll merge this in. |
|
Side-effect of ?w=1, you don't get the blue + (plus) sign which lets you do inline comments in a PR review. GRRR. |
|
Ok, this is back in sync with |
The entire codemap system is convoluted offers very little real benefit over the standard
require. It ends up masking call stacks and wrapping everything in at least one additional closure and makes it impossible for standard tooling (i.e. code hinting) to parse module exports properly across the application. It loads, parses and executes every module in the application, regardless of whether or not it needs to, at application initialization in order to build out it's internal module library in the codemap. It's additionally impossible to tell where any given module actually resides in the folder structure.I apologize for the size of this PR, but there wasn't a way to remove the codemap without touching almost every file. Additionally, since a function closure was removed from almost every file, the indentation level for nearly everything changed and Github isn't great about differentiating whitespace changes.
I expect there'll be some feedback/discussion for this. Let me know if there's anything I can do to make the PR easier to manage.