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

Divide core.helpers monolith #5959

Closed
wants to merge 6 commits into from
Closed

Divide core.helpers monolith #5959

wants to merge 6 commits into from

Conversation

eeiaao
Copy link

@eeiaao eeiaao commented Jan 6, 2019

Almost for three years core.helpers accumulated various kind of helpers and became a monolith. Separating helpers by their semantics should improve project readability and make easier to extend and maintain it.

First commits are aimed to prevent circular dependencies that are unavoidable due to the last one commit.

During working on port for Qt I was found that things like:
var helpers = require('../helpers/index');
module.exports = function() {
helpers.configMerge = ...
aren't allowed, that is, it's impossible to extend helpers in this way, so this part of a project is a blocker for a port.

@eeiaao
Copy link
Author

eeiaao commented Jan 6, 2019

Just found #5957... Guess it's worth to collaborate.

@simonbrunel
Copy link
Member

Thanks @eeiaao for your contribution.

Note that this PR generates tons of breaking changes since you moved most helpers under a new namespace, which will break external code (plugins and user projects) and in this case, we need to maintain deprecated aliases. However, we don't necessary want to move all helpers in a new namespace because it generates more code but also doesn't always make sense (for example Chart.helpers.isNumber should stay as it is).

There is also methods we don't want public anymore, so we should deprecate and make them private. For example, all the DOM related methods should move in platform.dom.js with alias but no new public API (meaning there should be no helpers.dom.js).

I initiated this split of helpers (#4419) but left most of the existing ones in core.helpers.js to give us a chance to select which ones we really want to keep as part of the public API, cleanup them and write unit tests and deprecate the other ones. For example configMerge and scaleMerge are not "helpers" but complex logic proper to our main controller and should certainly be moved in core/core.controller.js with a deprecated aliases.

Reviewing such big PR is really complicated, especially if we are going to iterate on which methods we want a new namespace or not and which ones we want to deprecate. I also have work in progress on that exact same task as part of #4478 and these changes massively conflict with it (but of course, you couldn't know about that).

Out of curiosity, why do you need these changes to port Chart.js to Qt? why don't you use the generated UMD build and override methods via Chart.* (something like this project). I think it's a simpler approach than trying to load the CJS sources (QML doesn't support require()).

@eeiaao
Copy link
Author

eeiaao commented Jan 8, 2019

@simonbrunel thanks for detailed explanations. I've totally forget about backwards compatibility with external code.

Recently I found that these changes aren't necessary at all (I just missed one file to include in QML resources).

The idea is to wrap the whole library in QML. The using of UMD builds will make maintenance painful since it will force to make builds everytime when some PR gets merged. Despite that QML itself doesn't support require(), there is a library that makes it possible (https://github.com/evg656e/requirify).

I spent a day to work on this idea and finally found that qml's Canvas has extremely slow implementation in comparison with any browser's one. Also there is another bottleneck - V4 JS engine.

So, this PR has to be rejected.

@eeiaao eeiaao closed this Jan 8, 2019
@simonbrunel
Copy link
Member

The using of UMD builds will make maintenance painful since it will force to make builds everytime when some PR gets merged.

You shouldn't depend on our master branch but from one of our tags which contain pre-built files.

Canvas has extremely slow implementation in comparison with any browser's one.

I would not say "extremely" because when using OpenGL, it's quite fast (maybe faster than browsers). You may also hit an issue with drawing lines with width > 1. Try to set renderTarget: Canvas.FramebufferObject to see if it makes any difference.

Also there is another bottleneck - V4 JS engine

That's right, but we are trying to keep our code compatible and optimized to target this kind of engine.

Did you try https://github.com/simonbrunel/qtchartjs? It definitely needs more work, tests, etc.. but I think it's a good start to integrate Chart.js in QML. Feel free to submit PR to that qtchartjs repository if you think it worth the effort.

@eeiaao
Copy link
Author

eeiaao commented Jan 11, 2019

You shouldn't depend on our master branch but from one of our tags which contain pre-built files.

What should I do whether I'd like to add some functionality that is not currently supported? Using Qt I don't ever need to use moment.js' localizations at all, since it can be done on QML side... So, going to add new property to time scale's options (consider it a formatLabel function), which can be used to format date-time labels instead of moment.js. Obviously it's a dread idea, but it makes my tasks feasible. I have to somehow keep this code current until changes that I needed appeared in main repo. However it may happen never, as I can see there is a lot of work made on the way to get rid of moment.js dependency and work is still in progress (I really appritiate it). This is an example of what I called a 'painful maintenance' in the previous message. I just don't want to make another dead fork. There is a way to keep an original library code wrapped and it's possible to make builds from it. I guess it much preferred than depend on pre-builds.

Try to set renderTarget: Canvas.FramebufferObject to see if it makes any difference.

It leads to black screen instead of Canvas on Android. Have no idea how and when it will be fixed.

Did you try https://github.com/simonbrunel/qtchartjs?

Have not seen. It would be interesting. Comparing to my port, it has different project structure, options/data updation approach and animations mechanism. Have to find a time to publish...

Anyway, I totally disagree with 'UMD builds' approach. This should be discussed.

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.

2 participants