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

TypeError: window.Dashboard.revive_payload is not a function #241

Closed
svilupp opened this issue Nov 20, 2023 · 28 comments
Closed

TypeError: window.Dashboard.revive_payload is not a function #241

svilupp opened this issue Nov 20, 2023 · 28 comments
Assignees

Comments

@svilupp
Copy link
Contributor

svilupp commented Nov 20, 2023

First, thank you for the awesome package.

I suspect that this PR is breaking for all Stipple apps running on previous versions in ENV=PROD.

The reason is that our mini app has stopped working after the release with the following error:

app/:228 TypeError: window.Dashboard.revive_payload is not a function
    at window.parse_payload (app/:386:22)
    at app/:213:14
    at WebSocket.<anonymous> (app/:157:11)

The local version that uses corresponding JS helper files works fine. The PROD version that grabs scripts from GenieFramework CDN throws the above error in the browser console.

Is there a way to add an alias to be backwards compatible?
It's a minor version release, so I wouldn't expect it to be breaking.

@essenciary
Copy link
Member

@hhaensel can you please check this - if you can fix quickly great, otherwise we need to have a new release to rollback the change until it's backwards compatible.

@essenciary
Copy link
Member

I will also take a look as the CDN should use the versioned script, not the master.

@svilupp
Copy link
Contributor Author

svilupp commented Nov 20, 2023

Thank you for the quick response.

Re. versioning, agreed. But I misspoke -- this was a patch version difference (eg, I was on 0.27.0 and the breaking release was 0.27.18)

Since I updated to the latest Stipple, I noticed another bug in the Sessions from some recent changes. I'll open the issue with the git blame.

@hhaensel
Copy link
Member

@svilupp May I ask, when revive_payload() is called? Are you calling it in private scripts?
I removed that function and replaced it by adding a simpler reviver to Genie's revivers.

If you provide a snippet of your script, I could either propose a solution (most probably it can be simply left out) or I could bring back the function in a new version of Stipple.
As that function was not part of the official API, I considered the release non-breaking. Sorry that this caused some trouble for you.

@hhaensel
Copy link
Member

Oh, I begin to understand. @essenciary is probably right that the CDN version and the Stipple version don't fit.
Then Stipple's watchers.js has changed.
@essenciary I was never involved in releasing CDN versions of the assets. How is that done?

@svilupp
Copy link
Contributor Author

svilupp commented Nov 20, 2023

I am not calling the function directly. It’s a normal Stipple dashboard on Stipple 0.27.0, which calls revive_* as part of the JavaScript codegen.

It was an issue only when running in prod env, when the scripts are sourced from CDN, so perhaps the issue is simply that it wasn’t versioned.

My Stipple locally created model that required the function, but it was no longer available on the CDN.

——
Simply updating to the latest Stipple solves the problem, but not everyone can do it / it might break some dashboards.

@hhaensel
Copy link
Member

hhaensel commented Nov 20, 2023

I just checked that there are, indeed, different (and correct) versions of watchers.js on

https://cdn.statically.io/gh/GenieFramework/stipple.jl/v0.27.17/assets/js/watchers.js

and

https://cdn.statically.io/gh/GenieFramework/stipple.jl/v0.27.18/assets/js/watchers.js

I suspect that you have deved Stipple and are running from master. In that case Stipple will call

https://cdn.statically.io/gh/GenieFramework/stipple.jl/master/assets/js/watchers.js

and you will run in the difficulties above.
If you still want to run a deved version you can, but you have to manually set the correct version number, e.g.

using Pkg
ver = filter(x-> x.second.name == "Stipple", Pkg.dependencies()) |> x -> first(x)[2].version
Genie.Assets.assets_config!(Stipple, version = string(ver))

@hhaensel
Copy link
Member

@essenciary could it be that we always use version = "master"?
If so it's our job to include above snippet in Stipple.jl ...

@hhaensel
Copy link
Member

We might add

function version_string(package::String)::String
    endswith(package, ".jl") && (package = String(package[1:end-3]))
    pkg_dict = filter(x -> x.second.name == package, Pkg.dependencies())
    isempty(pkg_dict) ? "master" : string(first(pkg_dict)[2].version)
end

version_string(m::Module) = version_string(String(nameof(m)))

function Genie.Assets.AssetsConfig(m::Module)
    package = "$(nameof(m)).jl"
    version = string(package_version(m))
    Genie.Assets.AssetsConfig(; package, version)
end

function Genie.Assets.AssetsConfig(package::String)
    version = version_string(package)
    Genie.Assets.AssetsConfig(; package, version)
end

so that we then can

Genie.Assets.AssetsConfig("Stipple.jl")
# or
Genie.Assets.AssetsConfig(Stipple)

That would be breaking, though, because the files would no longer reside in master but in the respective branch

@hhaensel
Copy link
Member

@svilupp
I've fixed the issue and brought back revive_payload() in watchers.js.
Thanks for reporting.

@essenciary We should consider the above changes for the next minor Stipple release ...

@svilupp
Copy link
Contributor Author

svilupp commented Nov 21, 2023

@essenciary could it be that we always use version = "master"? If so it's our job to include above snippet in Stipple.jl ...

FWIW, I believe that was driver of my issue. I didn’t clock that. It’s always set for version=“master”: https://github.com/GenieFramework/Genie.jl/blob/2128aa0147770f5944af4b46d7a902ae88a24083/src/Assets.jl#L25C10-L25C10

I didn’t dev the package, that was the behaviour on 0.27.0 that was added as a normal package.

@essenciary
Copy link
Member

Yes, using master is the issue.
@hhaensel adding the version_string part to Genie.Assets and I'll tag a release.

@essenciary
Copy link
Member

OK - I pushed on master... Not sure how to test the tagged assets without actually tagging the Genie release first. So I'll do that.

@essenciary
Copy link
Member

@hhaensel thank you very much for the fix! ♥️

@hhaensel
Copy link
Member

hhaensel commented Nov 21, 2023

You need to modify:

isa(package, Module) && (package = string(package))

to

isa(package, Module) && (package = String(nameof(package)))

because string(Stipple.JSON) == "Stipple.JSON3", while String(nameof(Stipple.JSON3) = "JSON3"
That's why I wrote two methods.

EDIT: In our standard use cases it will probably not play a role, but if people might use it for other purposes it would be nice if the function gives a correct value.

@essenciary
Copy link
Member

@hhaensel got it! I'll update and release

@essenciary
Copy link
Member

@svilupp can you please update to latest Genie and Stipple and check if everything works as expected? You should now be getting versioned assets.

@svilupp
Copy link
Contributor Author

svilupp commented Nov 21, 2023

Thank you for the quick turnaround.

It all worked fine on Stipple.jl 0.27.18.
When I upgraded Genie and Stipple to the latest, it stops working.

I get the following errors:

app/:331

GET https://cdn.statically.io/gh/genieframework/stipple.jl/0.27.19/assets/js/keepalive.js net::ERR_ABORTED 404 (Not Found)

Scripts seem to be properly injected into the main js file:

<script src="https://cdn.statically.io/gh/genieframework/stipple.jl/0.27.19/assets/js/underscore-min.js"></script>
<script src="https://cdn.statically.io/gh/genieframework/stipple.jl/0.27.19/assets/js/vue.min.js"></script>
<script src="https://cdn.statically.io/gh/genieframework/stipple.jl/0.27.19/assets/js/stipplecore.js"></script>
<script src="https://cdn.statically.io/gh/genieframework/stipple.jl/0.27.19/assets/js/vue_filters.js" defer></script>
<script src="https://cdn.statically.io/gh/genieframework/stipple.jl/0.27.19/assets/js/watchers.js"></script>
<script src="https://cdn.statically.io/gh/genieframework/stipple.jl/0.27.19/assets/js/keepalive.js" defer></script>
<script src="https://cdn.statically.io/gh/genieframework/stippleui.jl/0.22.10/assets/js/quasar.umd.min.js"></script>
<script src="https://cdn.statically.io/gh/genieframework/stippleplotly.jl/0.13.11/assets/js/plotly2.min.js"></script>
<script src="https://cdn.statically.io/gh/genieframework/stippleplotly.jl/0.13.11/assets/js/resizesensor.min.js"></script>
<script src="https://cdn.statically.io/gh/genieframework/stippleplotly.jl/0.13.11/assets/js/lodash.min.js"></script>
<script src="https://cdn.statically.io/gh/genieframework/stippleplotly.jl/0.13.11/assets/js/vueresize.min.js"></script>
<script src="https://cdn.statically.io/gh/genieframework/stippleplotly.jl/0.13.11/assets/js/vueplotly.min.js"></script>
<script src="https://cdn.statically.io/gh/genieframework/stippleplotly.jl/0.13.11/assets/js/sentinel.min.js"></script>
<script src="https://cdn.statically.io/gh/genieframework/stippleplotly.jl/0.13.11/assets/js/syncplot.js"></script>

However, they do not seem available on the CDN.

I get the following when I try to open the files directly:

Not Found

Remote or internal content not found. Back to homepage or Ask the Community.`

@essenciary
Copy link
Member

Oh ugh, the CDN does not support URLs with Git tags.

@essenciary
Copy link
Member

OK, it works but it needs the v as in vX.Y.Z

@essenciary
Copy link
Member

@svilupp releasing new patch versions. Sorry for the issues and thanks for the info.

@essenciary
Copy link
Member

Woohoo! Everything looks good with the current releases.

image

@hhaensel
Copy link
Member

https://cdn.statically.io/gh/genieframework/stipple.jl/master/assets/js/watchers.js

is not yet updated, but

https://cdn.statically.io/gh/genieframework/stipple.jl/v0.27.19/assets/js/watchers.js

is already available.

see above 😉 it had the 'v' already.

The master is now updated, so any version should currently run with master as well

@essenciary
Copy link
Member

@hhaensel Mmmm yes... but not the version I initially released...

@hhaensel
Copy link
Member

master should be the current master, right?

@svilupp
Copy link
Contributor Author

svilupp commented Nov 22, 2023

FWIW, I can confirm that the latest master works well. This issue can be closed from my perspective.

@essenciary
Copy link
Member

@svilupp thanks for confirming, closing.

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

No branches or pull requests

3 participants