Moerill / fvtt-chat-notifications

MIT License
9 stars 6 forks source link

[BUG] Chat Notifications v1.2.0 does not work with Foundry 0.7.5 #5

Closed cgplayer2000 closed 3 years ago

cgplayer2000 commented 3 years ago

Bug Description: The module doesn't repeat the chat messages elsewhere on the screen. The module does not appear in the Configure Settings section to allow it to be configured.

To Reproduce:
Update Foundry to latest stable 0.7.5.
Install 'Hey Listen - Chat Notifications' module.
Check the Hey Listen module in the 'manage modules' section. After refreshing the app, check the 'configure settings' section. You should see it, and be able to adjust some configurations. There will be no evidence of the module.

No relevant errors show up in the console.

The expected behavior of the module is that, once enabled, that the module would repeat messages sent to chat to a user-defined portion of the main screen. Though the module is installed, and activated, it does not appear in the module settings and no chat messages appear in the user-defined area of the main screen.

There are no known or identified module incompatibilities. I ran 'Find the Culprit' several times with no identified incompatibilities.

image

I also checked this situation in the latest version of Edge, and the same situation prevailed.

Moerill commented 3 years ago

Thanks for your bug report!

I can not reproduce it, for me everything works fine and as expected.
You are using the forge as hosting service, right?

cgplayer2000 commented 3 years ago

That’s correct. Ah, this might be a Forge artifact. Hmmm.

Sent from Mailhttps://go.microsoft.com/fwlink/?LinkId=550986 for Windows 10

From: Moerillmailto:notifications@github.com Sent: Thursday, October 22, 2020 16:44 To: Moerill/fvtt-chat-notificationsmailto:fvtt-chat-notifications@noreply.github.com Cc: cgplayer2000mailto:cgplayer@hotmail.com; Authormailto:author@noreply.github.com Subject: Re: [Moerill/fvtt-chat-notifications] [BUG] Chat Notifications v1.2.0 does not work with Foundry 0.7.5 (#5)

Thanks for your bug report!

I can not reproduce it, for me everything works fine and as expected. You are using the forge as hosting service, right?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMoerill%2Ffvtt-chat-notifications%2Fissues%2F5%23issuecomment-714751234&data=04%7C01%7C%7C7f80a63a90ec4ac7e31c08d876cb4b46%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637389962791613938%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Kg5vENil5Om4ZpGGq9PB%2F%2BrXmRRtIczKD9W%2FicX148c%3D&reserved=0, or unsubscribehttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FACSMOH6DFEBENJHSDAM4THLSMCKTLANCNFSM4S3WKRJA&data=04%7C01%7C%7C7f80a63a90ec4ac7e31c08d876cb4b46%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637389962791623921%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=v%2BF%2BTfzmP9k%2F8iF4W4NFvAclI15ZXU0L7pLDaWUxHYg%3D&reserved=0.

Moerill commented 3 years ago

i'm asking the forge creator to check whether or how to tackle this (if its a forge thing^^)

cgplayer2000 commented 3 years ago

Cool, thanks. Kakaroto’s a good dude.

Sent from Mailhttps://go.microsoft.com/fwlink/?LinkId=550986 for Windows 10

From: Moerillmailto:notifications@github.com Sent: Thursday, October 22, 2020 16:48 To: Moerill/fvtt-chat-notificationsmailto:fvtt-chat-notifications@noreply.github.com Cc: cgplayer2000mailto:cgplayer@hotmail.com; Authormailto:author@noreply.github.com Subject: Re: [Moerill/fvtt-chat-notifications] [BUG] Chat Notifications v1.2.0 does not work with Foundry 0.7.5 (#5)

i'm asking the forge creator to check whether or how to tackle this (if its a forge thing^^)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMoerill%2Ffvtt-chat-notifications%2Fissues%2F5%23issuecomment-714752978&data=04%7C01%7C%7C8108e47afeaf451af83708d876cbc5da%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637389964845542395%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=0J6J7f4wzlw7fbx08pZjH15Qln5dOdZAn%2BT20Js6GIY%3D&reserved=0, or unsubscribehttps://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FACSMOH7QM4267WN5ON2OSO3SMCLAJANCNFSM4S3WKRJA&data=04%7C01%7C%7C8108e47afeaf451af83708d876cbc5da%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637389964845552390%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=l1%2ByWiWsOp0hFmAltR%2FYU0yER61FKtFTvO6YYLWMnXk%3D&reserved=0.

WildBillLives commented 3 years ago

Just a confirmation for cgplayer2000 's post above...neither the configure tool nor the chat messages show up when running under The Forge when using Foundry 0.75.

cgplayer2000 commented 3 years ago

https://gitlab.com/foundrynet/foundryvtt/-/issues/3797 Through some additional debugging that @Alarich led me through, it looks like the problem might be related to this issue.

[cid:image003.png@01D6A898.52E5DE20] I was just curious what this was related to. That led to this:

[cid:image002.png@01D6A898.3AF88830]

Which in turn led to this.

[cid:image001.png@01D6A898.28B8E6B0]

Hope this helps.

Sent from Mailhttps://go.microsoft.com/fwlink/?LinkId=550986 for Windows 10

From: WildBillLivesmailto:notifications@github.com Sent: Thursday, October 22, 2020 17:11 To: Moerill/fvtt-chat-notificationsmailto:fvtt-chat-notifications@noreply.github.com Cc: cgplayer2000mailto:cgplayer@hotmail.com; Authormailto:author@noreply.github.com Subject: Re: [Moerill/fvtt-chat-notifications] [BUG] Chat Notifications v1.2.0 does not work with Foundry 0.7.5 (#5)

Just a confirmation for cgplayer2000 's post above...neither the configure tool nor the chat messages show up when running under The Forge.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://eur05.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMoerill%2Ffvtt-chat-notifications%2Fissues%2F5%23issuecomment-714764413&data=04%7C01%7C%7C0e1cc2c1c4de4c15823908d876ceff0d%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637389978691482239%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Et%2BqnnxlFBFyC49KF8v%2FhJItqRXob5fjy2pKLmZoqNo%3D&reserved=0, or unsubscribehttps://eur05.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FACSMOH7TLABRLU745CF5OBDSMCNWXANCNFSM4S3WKRJA&data=04%7C01%7C%7C0e1cc2c1c4de4c15823908d876ceff0d%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637389978691482239%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=4NNbcb9%2ByNrX7CTu8ppPVSa%2Fpc8QEjUxllQslNSQybc%3D&reserved=0.

Moerill commented 3 years ago

That path is changed and fixed. BUT it could be the root cause for it not working in the forge... Still waiting for Kakaroto to react!

cgplayer2000 commented 3 years ago

Cool, thanks!

Sent from Mailhttps://go.microsoft.com/fwlink/?LinkId=550986 for Windows 10

From: Moerillmailto:notifications@github.com Sent: Thursday, October 22, 2020 17:32 To: Moerill/fvtt-chat-notificationsmailto:fvtt-chat-notifications@noreply.github.com Cc: cgplayer2000mailto:cgplayer@hotmail.com; Authormailto:author@noreply.github.com Subject: Re: [Moerill/fvtt-chat-notifications] [BUG] Chat Notifications v1.2.0 does not work with Foundry 0.7.5 (#5)

That path is changed and fixed. BUT it could be the root cause for it not working in the forge... Still waiting for Kakaroto to react!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMoerill%2Ffvtt-chat-notifications%2Fissues%2F5%23issuecomment-714774013&data=04%7C01%7C%7Cb3e20aa5bee04a7bb0bf08d876d1e9c6%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637389991220224121%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=fdGsZ5E3WLTZ%2FrZiuvV4n6OSCUmjwm%2BiYNk3CNrMsMU%3D&reserved=0, or unsubscribehttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FACSMOHZLAHKJEQH4YADUJ5LSMCQEZANCNFSM4S3WKRJA&data=04%7C01%7C%7Cb3e20aa5bee04a7bb0bf08d876d1e9c6%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637389991220234110%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Qi9449SNmXGqEaUQjAzRE%2FU29nH9EnTlWjMKeh5IJ1c%3D&reserved=0.

kakaroto commented 3 years ago

Thanks for the bug report. Yep, there was definitely an issue from the Forge's side, the greensock library wasn't being served from the CDN when using absolute paths to it. I've added it now to the CDN, but this brings up the issue of the two conflicting greensock libraries between 0.6.x and 0.7.x and if you're doing an esmodule import instead of specifying the file in the module.json's scripts, then that's bound to cause issues eventually, because Foundry's design about the greensock inclusion says to specify the library files you need in the scripts of module.json, and foundry will take care to include it automatically. Bypassing that system and using a relative import in your js files is bound to have consequences.

Note also that you're now using greensock paths for 0.7.x, but your module.json says minimumCompatibleVersion is 0.6.6 but it should be 0.7.5

Moerill commented 3 years ago

Thanks for replying!

oh i thought i updated the compatibleCoreVersion to 0.7.4 <_<

are you sure about gsap still having to be included as script file in the module.json? i thought this kinda "old" news now, since the new gsap version is bundled as esmodule and not as "pure" script, like before. What kind of consequences is this bound to have? (Besides the obvious one with the forge)

Moerill commented 3 years ago

fixed the compatible and minimumcoreversion in the module.json.

kakaroto commented 3 years ago

I haven't seen anything about it having to be included differently, so I assume that's still the proper way of doing it and the issue https://gitlab.com/foundrynet/foundryvtt/-/issues/3797 from last week says "module developers who were using Greensock must update their manifest files to point to the new library versions"

I'm not well versed in esmodule stuff, but... are you saying that greensock is now only available as esmodules and those who are not using esmodules (and who refuse to use them because &#!&#!&#!#! I don't want to) cannot use greensock anymore since 0.7.4 and that loading it from the scripts tag wouldn't work anymore ? Honestly, if that's the case, I don't understand why Atro couldn't leave the dist folder there at the same time, it's not like it was hurting anyone (also, now you load all.js which then loads a bunch of other js files through import which then load others.. etc.. and your one file request has transformed into 29 requests!!!! causing both a significant slowdown in loading times for the user and increasing load on servers.. and that's why I hate esmodules.... )!

Anyway, the issue is that you load /scripts/greensock/esm/all.js and that means you're loading an absolute hardcoded path (in this case https://assets.forge-vtt.com/scripts/greensock/esm/all.js), which if it changes again in the future, Foundry isn't able to automatically map the old path to a new one as needed, which it would have been possible if you had just specified the path you wanted in the module.json (since it could have decided "greensock/esm/all.js" maps to a different actual url... but seeing how Atro didn't bother to do that for 0.6.x=>0.7.4 he might never take advantage of that ability and I think his stance is "let the devs fix their own modules" rather than providing a stable backwards-compatible API).

It also has the effect that your module can't work by being on a CDN that is served from a different URL because that path would only work if the module is loaded from the exact same url as the game itself, and finally (https://mygameurl.forge-vtt.com instead of https://assets.forge-vtt.com), if one module loads from the game's url and yours loads from the CDN url, then the same greensock library will be loaded twice by chrome because they'd be different urls so greensock would overwrite itself or conflict with itself, or initialize things twice, which could cause undefined behavior. For now, you can load greensock from the CDN because I made the files available there, but if suddenly there's a version conflict, then that's going to be a problem (i.e: a module using absolute paths to load 0.6.x greensock and one using 0.7.4 greensock.. that can't work because it's same urls, but different versions)

Anyway... looks like I need to figure out this esmodule thing :(

Moerill commented 3 years ago

uff - the networking stuff really is not my strong suite. Though isn't the 26 vs 1 requests mainly only an issue since fvtt still uses HTTP1 and not HTTP2?

on another note: i (just) tried including the GSAP library using the module.json and it doesn't seem to work. (i haven't tried including every file, but only with the collection ones, like all and gsap-core) But the absolute path is not really an option as well, since it won't work with people using foundry using the routePrefix option. So the alternative would be to use a relative path.. Would this work with the forge with how you set it up now?

And ye the version conflict may happen, if Atro keeps on updating gsap now.

kakaroto commented 3 years ago

uff - the networking stuff really is not my strong suite. Though isn't the 26 vs 1 requests mainly only an issue since fvtt still uses HTTP1 and not HTTP2?

Not really, no. With http1, they have to be done 1 after the other, with the SSL handshake and HTTPS re-establishment being done every time which add latency to each request. With http2, they can be done through simultaneous streams, which makes it faster, BUT, you still need to send the HTTP request with all the headers and receive the HTTP response with all its headers, which can be quite a big overhead, you're also limited to 100 streams at a time, and I've seen some worlds with over 2000 requests being performed (mostly because of things like this, + dnd5e esmodules + other modules, etc... it adds up really quickly), so you have to do 100 at a time and wait each time, so the world loads slowly... and well, server side, whether it's http1 or http2, it makes zero difference, each request has to be handled separately, taking up CPU and RAM to handle, each file has to be fetched from disk separately as well (causing a lot of disk seek which is where the largest part of latency occurs when it comes to small files), etc... so overall, it's just really really bad and is why I hate it so much.

But the absolute path is not really an option as well, since it won't work with people using foundry using the routePrefix option.

Right! Should have thought of that... yes, using absolute paths would fail for anyone using routePrefix. Adding the include in the module.json should allow for that. But like you said, it's not working, I think because it's esm only and Atro removed the dist folder, which I think means it won't work! What you can probably do is add "esmodules": ["greensock/esm/all.js"] and it would work, rather than adding it to the scripts field. I'll file an issue with him for why does it force devs to use esm now.

The Forge should handle relative paths, yeah.. But I just checked, and it won't for this specific use case 🤦‍♂️ I'll fix that up right now. The same file should be resolved if you try /scripts/greensock/esm/all.js or /bazaar/greensock/esm/all.js or /bazaar/modules/scripts/greensock/esm/all.js or /bazaar/systems/scripts/greensock/esm/all.js. Thanks for making me notice this other use case... :)

Moerill commented 3 years ago

"esmodules": ["greensock/esm/all.js"] and it would work

i tried that, its loading the scripts (according to dev tools).. buut i can't really access the functionality, since its just exporting them to "nowhere"? (Don't know how the esm stuff from foundry (or even the browser) works :grimacing: )

disk separately as well (causing a lot of disk seek which is where the largest part of latency occurs when it comes to small files),

i kinda expected for the latency of talking with the server to be waaay worse than disk seek :sweat_smile:
Kinda thought the same for all the resource consumption when serving stuff - expected that to be very minor compared to other stuff (like the databases).
But you kinda have a point there, that i may return to doing "regular concatenated scripts" (like foundry.js) where possible.
Only including gsap/all cause it was more easy than searching for the correct files i need :sweat_smile:

Thanks for making me notice this other use case... :)

happy to help! :D

Moerill commented 3 years ago

The Forge should handle relative paths, yeah..

ok doing relative paths for the time being - until we possibly have a module.json solution. Don't know why i haven#t done it before, isn't import { TimelineMax } from '../../../../scripts/greensock/esm/all.js'; just a beautiful import? <_<

(Atro should just add gsap to the global namespace and use it himself instead of all the jquery animations! would be soooo much better)

kakaroto commented 3 years ago

i tried that, its loading the scripts (according to dev tools).. buut i can't really access the functionality, since its just exporting them to "nowhere"? (Don't know how the esm stuff from foundry (or even the browser) works 😬 )

🤦‍♂️ of course, yeah, I see how that wouldn't work... oh well, how can I say one more time that I hate esmodules 🤣

i kinda expected for the latency of talking with the server to be waaay worse than disk seek 😅

Lol, yes, of course, but I meant it's the largest part of the latency on the server side in handling the request... i.e: if it takes it 10ms to handle the request (from the moment it starts parsing the http request headers to the moment it starts sending http response headers), 8ms of it will be I/O waiting for the disk :P

isn't import { TimelineMax } from '../../../../scripts/greensock/esm/all.js'; just a beautiful import? <_<

Soooo beautiful! Don't forget to fix that path if you need to import it from a file within a different subdir! And I just love how it imports sooooooo much stuff, just to drop everything and use only TimelineMax... that's just beautiful! :P

Moerill commented 3 years ago

Soooo beautiful! Don't forget to fix that path if you need to import it from a file within a different subdir! And I just love how it imports sooooooo much stuff, just to drop everything and use only TimelineMax... that's just beautiful! :P

Ye, so awesome! thats the part i hate the most about esm modules - fixing the path when i decide to change some folder structure..

I'll file an issue with him for why does it force devs to use esm now.

Especially since he doesn't do esm for foundry ! :angry: :P (i do like the many small files for developing and the "sandboxy" environment they provide)

just checked the gsap github, there are still dist files, so adding that instead of the esm files, could make things easier, let them be loaded as scripts again and solve most issues here.

btw i think i can close this issue for now? (doing so, until someone complains about me doing it )

kakaroto commented 3 years ago

hehe, yeah, we're just chatting, it can be closed. I've filed the observations from here as a comment to the original issue : https://gitlab.com/foundrynet/foundryvtt/-/issues/3797

I definitely like the concept of developing in small files, I'm not a fan of the module isolation (the sandbox) but only because of cross module imports which are (were) messing up with the CDN, but I do like it in concept. For my own stuff, I just develop in small files and concat them with gulp.. although Furnace has many small files all listed in the scripts field of the manifest.

kakaroto commented 3 years ago

Just FYI, it turned out that the missing dist directory was an error and 0.7.6 will have the directory restored, so it can be included in the scripts field again 👍