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

Add a service-worker example #523

Merged
merged 1 commit into from
Sep 7, 2020

Conversation

johnsonw
Copy link
Contributor

Service worker can be integrated into the seed framework. This example
demonstrates the following:

  1. Use service worker to cache all assets (including the generated wasm file).
  2. Register the service worker.
  3. If the service worker is not yet activated, an even listener will be registered, waiting for the
    state to reach "activated".
  4. When the state reaches "activated", the Notification object will request permission for notifications.
  5. If permission is granted, the PushManager will subscribe to the service using an example vapid key.
  6. Finally, a PushSubscription will be returned, containing the information that can be passed to a
    notifcation back-end server.

Signed-off-by: johnsonw william.c1.johnson@gmail.com

@johnsonw johnsonw force-pushed the service-worker-example branch 4 times, most recently from 92ea70e to 4986d0f Compare August 30, 2020 05:44
@MartinKavik
Copy link
Member

Could we remove npm/yarn and workbox-cli from the example?

  • Examples should demonstrate basic concepts and it looks like Workbox hides some abstractions and contains code generators inside. (I see Workbox for the first time.)

  • We don't want to integrate JS tools into the repo. I wasn't able to run the example even when I have yarn installed. And it's often a time-bomb and all examples are used as integration tests both on contributors local machines and on CI pipeline - it would complicate and slow down the verification processes.

One of the big reasons why users migrate from JS frameworks to Seed are too complicated and unreliable JS tools, we shouldn't disappoint them.

@johnsonw
Copy link
Contributor Author

Sure, no problem. I'll take both workbox-cli and yarn out. Out of curiosity, cargo make start didn't run the example? What happened when you went to http://localhost:8000?

@MartinKavik
Copy link
Member

MartinKavik commented Aug 30, 2020

Out of curiosity, cargo make start didn't run the example?

image

The same result when I run the command directly from the example directory. The same for the the task inject_manifest. I'm not sure if it's a cargo-make bug.

Also there are some commands like rm -rf that would probably fail on "non-linux" platforms and terminals like Windows cmd or powershell. We should use only Rust tools or cross-platform scripts - ideally Duckscript - see, for instance,

seed/Makefile.toml

Lines 276 to 294 in 4387219

[tasks.for_each]
description = "Run chosen task for each example in its root. Ex: 'cargo make for_each build'"
script = [
'''
#!@duckscript
defined = is_defined 1
assert ${defined} "Wrong number of arguments! Correct example: 'cargo make for_each build'"
task = set ${1}
handle = glob_array examples/*/Makefile.toml
for path in ${handle}
example_root = dirname ${path}
echo Example root: ${example_root}
exec --fail-on-error cargo make --cwd ${example_root} ${task}
end
'''
]
.

I'll do another code review round once yarn/npm & workbox issue is resolved.

@johnsonw
Copy link
Contributor Author

Thanks for sharing. I've removed the package.json, yarn lock file, workbox-cli etc and will push again shortly.

@johnsonw
Copy link
Contributor Author

Just giving an idea of what the page looks like. This is a screenshot after I stopped the local server:
image

image

The files being cached:
image

@MartinKavik
Copy link
Member

MartinKavik commented Aug 30, 2020

Firefox (Windows) has a problem with the notification request:
image

Chrome shows a standard request without problems.

@MartinKavik MartinKavik mentioned this pull request Aug 30, 2020
@MartinKavik
Copy link
Member

Btw cargo make clean is a good idea, I've created a dedicated issue for the global alternative: #526.

@johnsonw
Copy link
Contributor Author

Yeah I figured it made sense to clean up the pgk directory. Not sure if you saw my update but I changed it to:

[tasks.clean]
command = "cargo"
args = ["clean", "--target-dir", "pkg"]

@johnsonw
Copy link
Contributor Author

Regarding the firefox error I believe they want a user gesture before subscribing so I'm adding that now. Should have another push shortly.

@johnsonw
Copy link
Contributor Author

Updates with firefox:
image

Copy link
Member

@MartinKavik MartinKavik left a comment

Choose a reason for hiding this comment

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

I've added some review comments.

Also please update CHANGELOG.md (I think there is a line "Added examples ..." - you can append your example).
And examples/README.md.

Let me know once you are ready for the next round 🙂

examples/service_worker/Cargo.toml Outdated Show resolved Hide resolved
examples/service_worker/Cargo.toml Outdated Show resolved Hide resolved
examples/service_worker/Cargo.toml Outdated Show resolved Hide resolved
examples/service_worker/README.md Outdated Show resolved Hide resolved
examples/service_worker/custom.js Outdated Show resolved Hide resolved
examples/service_worker/src/lib.rs Outdated Show resolved Hide resolved
examples/service_worker/src/lib.rs Show resolved Hide resolved
examples/service_worker/src/lib.rs Outdated Show resolved Hide resolved
examples/service_worker/src/lib.rs Outdated Show resolved Hide resolved
examples/service_worker/src/lib.rs Outdated Show resolved Hide resolved
@flosse
Copy link
Member

flosse commented Aug 31, 2020

off-topic / BTW:

One of the big reasons why users migrate from JS frameworks to Seed are too complicated and unreliable JS tools, we shouldn't disappoint them.

I totally agree!! 👍

@johnsonw
Copy link
Contributor Author

Thanks for the thorough review! I'm going through the changes now.

Copy link
Member

@MartinKavik MartinKavik left a comment

Choose a reason for hiding this comment

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

Another round finished!
Major problems should be eliminated once all comments are resolved. In the next round I'll focus on code refactoring / best practices.

Some issues not mentioned in discussions:

  • CHANGELOG.md and examples/README.md aren't updated if I'm not wrong (see my previous comment attached to "Request changes" comment.)
  • It would be nice to add a button "Clear cache" because it's a bit annoying when localhost constantly returns the cached example even if you run other examples.

examples/service_worker/service-worker.js Outdated Show resolved Hide resolved
examples/service_worker/service-worker.js Outdated Show resolved Hide resolved
examples/service_worker/src/errors.rs Outdated Show resolved Hide resolved
examples/service_worker/src/lib.rs Outdated Show resolved Hide resolved
examples/service_worker/src/lib.rs Outdated Show resolved Hide resolved
examples/service_worker/src/lib.rs Outdated Show resolved Hide resolved
examples/service_worker/src/lib.rs Outdated Show resolved Hide resolved
examples/service_worker/src/lib.rs Outdated Show resolved Hide resolved
examples/service_worker/src/lib.rs Outdated Show resolved Hide resolved
examples/service_worker/src/lib.rs Outdated Show resolved Hide resolved
@johnsonw
Copy link
Contributor Author

Thanks for the additional review. I'll address each item once I get off work.

@johnsonw
Copy link
Contributor Author

johnsonw commented Sep 1, 2020

I've addressed all by one comment (moving the initial call to the init function). The CHANGELOG.md and README.md are both updated as well. Regarding a button to clear the cache I'll see if I can add one tomorrow. I've been using the developer tools to clear out the cache while testing. Let me know what you recommend regarding the init function.

@MartinKavik
Copy link
Member

Blocked by johnsonw#1

Copy link
Member

@MartinKavik MartinKavik left a comment

Choose a reason for hiding this comment

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

Another round finished. Looks pretty good, everything works. I was focusing on style, consistency and cross-platform support. Good job!

Please squash commits when you are done. I rebase on master and commits like Fixups and more fixups would look ugly in the Seed commit history and it wouldn't help during debugging.

examples/service_worker/README.md Outdated Show resolved Hide resolved
examples/service_worker/README.md Outdated Show resolved Hide resolved
examples/service_worker/README.md Show resolved Hide resolved
examples/service_worker/frontend/src/lib.rs Outdated Show resolved Hide resolved
examples/service_worker/frontend/src/lib.rs Outdated Show resolved Hide resolved
examples/service_worker/frontend/src/lib.rs Outdated Show resolved Hide resolved
examples/service_worker/backend/src/main.rs Outdated Show resolved Hide resolved
examples/service_worker/backend/src/main.rs Outdated Show resolved Hide resolved
examples/service_worker/backend/src/main.rs Outdated Show resolved Hide resolved
examples/service_worker/frontend/src/lib.rs Outdated Show resolved Hide resolved
Service worker can be integrated into the seed framework. This example
demonstrates the following:

1. Use service worker to cache all assets (including the generated wasm file).
2. Register the service worker.
3. If the service worker is not yet activated, an even listener will be registered, waiting for the
   state to reach "activated".
4. When the state reaches "activated", the Notification object will request permission for notifications.
5. If permission is granted, the PushManager will subscribe to the service using an example vapid key.
6. Finally, a PushSubscription will be returned, containing the information that can be passed to a
   notifcation back-end server.

Signed-off-by: johnsonw <william.c1.johnson@gmail.com>
@MartinKavik MartinKavik merged commit 8f99195 into seed-rs:master Sep 7, 2020
@johnsonw johnsonw deleted the service-worker-example branch September 7, 2020 21:38
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.

None yet

3 participants