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

Regression: analytics selections malfunctioning #117



Copy link

closes #116

@Flix6x Flix6x added this to the 0.4.1 milestone May 4, 2021
@Flix6x Flix6x self-assigned this May 4, 2021
@Flix6x Flix6x added bug Something isn't working UI labels May 4, 2021
@Flix6x Flix6x requested a review from nhoening May 4, 2021 23:20
@Flix6x Flix6x marked this pull request as ready for review May 4, 2021 23:20
Copy link

nhoening commented May 5, 2021

So what is the difference between functions in flexmeasures.js and flexmeasures-modules.js? It isn't apparent to me at first glance...

Copy link

Flix6x commented May 5, 2021

One is a module and the other isn't. The bug had something to do with function definitions in js modules needing to be exported, but I don't know the fine details of it. So rather than exporting all function definitions in flexmeasures.js I think it is safer at this point to move out the recently introduced js content to a separate js module file, as I did in this PR. Otherwise I was worried about new side effects. Just to be clear, flexmeasures.js had not been a module type js file before, until we introduced the new (now to be separated) content.

Copy link

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still have a template which should switch over to the module:

± grep -r flexmeasures.js flexmeasures
flexmeasures/ui/templates/views/sensors.html:      import { subtract, thisMonth, lastNMonths } from "{{ url_for('flexmeasures_ui.static', filename='js/flexmeasures.js') }}";

Also, looking at that statement, it seems cleaner and more closer to the javascript module idea: import what you need. Shouldn't we do it the same way in base.html?

Other than that, I approve technically with this move.

I do, however, suggest better naming to move forward with this distinction (using modules has the main benefit to better organize the code):

  • flexmeasures.js -> "ui-behaviours.js" (I actually want to factor out feature-specific code from here, like for the control page [lines 212-287], I'll make a ticket)
  • flexmeasures-module.js -> "daterange-utils.js"

Copy link

Flix6x commented May 7, 2021

Good catch. The js module contents are actually not needed anywhere else currently, and I also prefer to import only what we need, so I'll stop importing our js module in our base.html. I'll also rename the module to "daterange-utils.js".

Just a note: this regression was caused by 1a238ef.

@Flix6x Flix6x requested a review from nhoening May 7, 2021 11:51
@Flix6x Flix6x merged commit 93ca7d5 into main May 7, 2021
@Flix6x Flix6x deleted the issue-116-Regression_analytics_selections_malfunctioning branch May 7, 2021 12:14
Flix6x added a commit that referenced this pull request May 7, 2021
Fixed regression and started more targeted loading of javascript functions from our own js modules.

* Create draft PR for #116

* Fix bug due to redefining older javascript content as a js module.

* Changelog entry

* More targeted loading of js functions from module

Co-authored-by: Flix6x <>
Co-authored-by: F.N. Claessen <>
Co-authored-by: Felix Claessen <>
Copy link

Flix6x commented May 7, 2021

@meeseeksdev Hello

Copy link

lumberbot-app bot commented May 7, 2021

Look at me, @Flix6x, I'm Mr. Meeseeks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
bug Something isn't working UI
None yet

Successfully merging this pull request may close these issues.

Regression: analytics selections malfunctioning
2 participants