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

Update papyros to version 2.0.0 #5204

Merged
merged 9 commits into from
Dec 6, 2023
Merged

Update papyros to version 2.0.0 #5204

merged 9 commits into from
Dec 6, 2023

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Nov 28, 2023

This pull request is related to the papyros build update dodona-edu/papyros#558

Papyros moved from a prebundled file to separate files. This allows us to avoid loading a lot of dependencies twice (eg codemirror).

Required changes:

  • load papyros css from css instead of from js
  • Avoid requiring commons chunk in serviceworker
  • Allow service worker to run on root with header headers['Service-Worker-Allowed'] = '/'

Future work: Avoid local duplicate of serviceworker
This would greatly simplify the setup, removing extra parameters from papyros and removing extra local configuration.
I haven't done this yet as I haven't yet found how to set the header headers['Service-Worker-Allowed'] = '/' to a specific asset.
Also, the number of changes in this papyros version was already quite large. So keeping this change for the future seemed sensible.

Bundle

Before 13.74MB
image

After 8.06MB
image
The tar with python code of 1.35MB should be added to this total
Bringing it to 9.41MB

@jorg-vr jorg-vr added the chore Repository/build/dependency maintenance label Nov 28, 2023
@jorg-vr jorg-vr self-assigned this Nov 28, 2023
@jorg-vr
Copy link
Contributor Author

jorg-vr commented Nov 29, 2023

finally figured out the issue in the current setup.
First of all everything works correctly in chrome.

In firefox the read requests do not get intercepted by the service worker. (while write request are still intercepted as expected)

The probable cause:

  • with the new build changes, the workers are separate packages instead of inline files.
  • these are served from /assets/... (as is every other asset
  • For some reason firefox decides they should trigger the serviceworker from the assets scope

I confirmed this issue by using a service worker on the /assets scope, which does intercept the read requests
This is not a solution as write requests are still scoped under the activity page

  • Prefered solution would probably be to run the serviceworker on the global scope, catching all.
    This might be feasible by simple headers, but I haven't been able to force rails to provide those headers on a specific asset

This might also have side effects when running multiple activities simultaneously, but I'll have to prove this theory in practice
update: I tested this, and there are no issues to report.

@jorg-vr jorg-vr added the deploy naos Request a deployment on naos label Dec 5, 2023
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Dec 5, 2023
@jorg-vr jorg-vr added the deploy naos Request a deployment on naos label Dec 5, 2023
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Dec 5, 2023
@jorg-vr jorg-vr added the deploy naos Request a deployment on naos label Dec 5, 2023
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Dec 5, 2023
@jorg-vr jorg-vr added the deploy naos Request a deployment on naos label Dec 5, 2023
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Dec 5, 2023
@jorg-vr jorg-vr marked this pull request as ready for review December 5, 2023 15:37
@jorg-vr jorg-vr requested a review from a team as a code owner December 5, 2023 15:37
@jorg-vr jorg-vr requested review from bmesuere and niknetniko and removed request for a team December 5, 2023 15:37
@jorg-vr
Copy link
Contributor Author

jorg-vr commented Dec 5, 2023

I'll publish the non beta version of 2.0.0 once this pr is approved

Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

I tested this in Safari, but received an error when entering a value for input:

RuntimeError: The service worker for reading input isn't working. Try closing all this site's tabs, then reopening. If that doesn't work, try using a different browser.

There also is a slight UI problem with the header:
image

@bmesuere
Copy link
Member

bmesuere commented Dec 6, 2023

Update: it did work after a hard refresh of the page

I would also remove the beta label from this feature.

@jorg-vr
Copy link
Contributor Author

jorg-vr commented Dec 6, 2023

The header layout is a separate bug already present in production, will fix in separate pr

I am unsure if I can avoid the need for a hard refresh...

@jorg-vr jorg-vr added the deploy naos Request a deployment on naos label Dec 6, 2023
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Dec 6, 2023
@jorg-vr jorg-vr added the deploy naos Request a deployment on naos label Dec 6, 2023
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Dec 6, 2023
@jorg-vr jorg-vr merged commit 6b21baf into main Dec 6, 2023
15 checks passed
@jorg-vr jorg-vr deleted the update/papyros branch December 6, 2023 14:24
@pdawyndt
Copy link
Contributor

pdawyndt commented Dec 6, 2023

I also needed a hard refresh on my Windows 10 machine with Google Chrome. Initially running a program got stuck after the first line of input was entered (interactive mode).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Repository/build/dependency maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants