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

[new package] autoupdate-polling #11034

Closed
wants to merge 9 commits into from
Closed

Conversation

delki8
Copy link
Contributor

@delki8 delki8 commented Apr 29, 2020

why

I created this package to address this part of the roadmap:

it would be great to find a way of making autoupdate less expensive

package README

autoupdate-polling is based on the popular autoupdate.
The goal is to provide a lightweight alternative that replaces the use of ddp by a polling mechanism that checks the server for a new version every 3 seconds. When it sees that a new version is available, it uses the reload package to gracefully save the app's state and reload it in place.

To make it work, while under development the server exposes a public endpoint that expects the client's archetype and returns the most recent build of the app's client.

production client bundle size for a --minimal meteor app

vanilla --minimal 17.8 KB
with autoupdate 48.3 KB + 30.5 KB
with autoupdate-polling 21.5 KB + 3.7 KB

caveats:

  • since autoupdate don't have any tests I decided to not write any new tests for autoupdate-polling

@Fen747
Copy link

Fen747 commented May 22, 2020

Great PR :)

3 seconds though seems a bit too aggressive imho, specially for apps serving a lot of users at the same time. In my personal experience, I have no app that break if the interval was higher.

Having the possibility to config / customize this would be the cherry top of the cake ;)

@delki8
Copy link
Contributor Author

delki8 commented May 22, 2020

Thank you for the incentive @Fen747. I wrote it to give the developer a comfortable option so he wouldn't need to manually refresh the pages while under development. So the 3 seconds were chosen to address this use case, more than that could make the dev hit the refresh button anyway or slow down development.

In fact, the whole package only works if Meteor.isDevelopment returns true because I was afraid to have too many production clients polling the server for potentially useless information. But that wouldn't be a problem if we had a way to config / customize this interval for each environment 😉.

I'll think about it.

@delki8
Copy link
Contributor Author

delki8 commented May 26, 2020

@Fen747 just to let you know that it's done 😄. The dev will be able to change the polling frequency by setting a public Meteor.setting.

}, delay);
};

if (!Meteor.isProduction || Meteor.settings.public.autoupdate_polling_time)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to use Meteor.settings.public?.autoupdate_polling_time to prevent errors when public settings is not set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this won't be necessary because Meteor.settings.public is guaranteed to always be defined according to the docs, even if there are no settings specified.

@@ -14,3 +14,4 @@ typescript # Enable TypeScript syntax in .ts and .tsx modules
shell-server # Server-side component of the `meteor shell` command
webapp # Serves a Meteor app over HTTP
server-render # Support for server-side rendering
autoupdate-polling # Provides a lightweight hot code push functionality
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this really be in the minimal skeleton?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tough one. On one hand HCP functionality is not strictly necessary to write an app, on the other hand HCP is an important part of the Meteor developer experience that up until now the minimal skeleton was missing off due to the size of the original autoupdate package.

I think it should be included in the minimal skeleton but I wouldn't mind leaving it off if you think it would be better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

is it acceptable as minimal to not have updates? I don't think so. So I vote to include it.

Copy link
Collaborator

@filipenevola filipenevola left a comment

Choose a reason for hiding this comment

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

Great work! I'm just suggesting a small adjustment.

}, delay);
};

if (!Meteor.isProduction || Meteor.settings.public.autoupdate_polling_time)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@delki8 I'm following a new pattern here, always defining package options on packages key.

Meteor.settings.public.packages['autoupdate-polling'].pollingInterval

can you change to follow this pattern?

see here one example https://docs.meteor.com/api/collections.html#mongo_connection_options_settings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. It's done 😄

@@ -14,3 +14,4 @@ typescript # Enable TypeScript syntax in .ts and .tsx modules
shell-server # Server-side component of the `meteor shell` command
webapp # Serves a Meteor app over HTTP
server-render # Support for server-side rendering
autoupdate-polling # Provides a lightweight hot code push functionality
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it acceptable as minimal to not have updates? I don't think so. So I vote to include it.

@filipenevola filipenevola added this to the Release 1.10.3 milestone Jun 8, 2020
@klaussner
Copy link
Contributor

Thanks for creating this PR, @delki8. The current autoupdate/hot-code-push implementation can definitely be improved, especially for development mode. I don't want to sound too negative but I have some reservations about this particular polling implementation. Most importantly:

As mentioned in #11034 (comment), a good polling interval depends on several factors: development vs. production, patience of the developer, type of application, etc., so the setting probably needs to be adjusted depending on the environment (for example, the three second interval would be too short if the application doesn't need to be reactive in production). However, Meteor has always strived to be a zero-configuration tool and I think it would be great if we could find a solution that follows this principle and covers more use cases (and personal preferences) without the need for configurability. Additionally, polling spams the network tab in browser development tools, which makes finding and debugging other requests more difficult.

One possible solution is to use SSE (server-sent events) instead of polling. SSE are supported by all major browsers and allow one-way communication from the server to the client. I've worked quite a bit on auto-reloading in #10505 (experimental error page reloads based on SSE), meteor/meteor-feature-requests#354 (concept for integrating reloading more deeply into the Meteor Tool), and meteor-auto-reload (similar to the package in this PR).

My experiments were for development only, so they wouldn't work for reloads in production without making some changes. However, it looks like development reloads were your main concern when you initially created this PR. Maybe we can combine some of these ideas here. What do you think? 🙂

Some additional points:

  • A lot of the code in the new package seems to be copied from autoupdate, which is not ideal for maintainability.
  • Client-only refreshes are not working. Meteor always restarts the server, even if only client files (such as CSS) are changed.

@delki8
Copy link
Contributor Author

delki8 commented Jun 11, 2020

Hello @klaussner. Thank you so much for the input and don't worry about how it sounds. I appreciate your discerning review and hope to address each of your concerns in my answer so we can continue talking about it.

  1. the setting probably needs to be adjusted depending on the environment
    @Fen747 had a similar concern and that's why I changed it to allow the users to change the polling time with a setting that could be different in production or development. check it here

  2. Meteor has always strived to be a zero-configuration tool
    autoupdate-polling will not demand any configuration unless the user wants to change the polling frequency or wants to use it to hot reload production code. I'm keeping these 3 seconds for simplicity's sake. They were chosen arbitrarily while I was testing and I thought they felt comfortable, but surely this default value could be changed.

  3. polling spams the network tab in browser development tools
    I couldn't agree more. I had to weight what would be worse for those --minimal devs: have to hit the refresh button every time or have a spammed network tab. Considering the polling time configuration option and the fact that users can simply uninstall the package, I still believe that giving them this lightweight option (with a spammed network tab as a tradeoff) is the right call.

  4. One possible solution is to use SSE
    I didn't know SSE and now that I'm reading about it, it sounds like a good alternative to polling. Since you had worked with it before, would you be willing to test an adaptation of autoupdate-polling to work with SSE? If we come up with a solution like this we probably would need to change the name of the package to something that doesn't reflect the underlying mechanism, maybe autoupdate-minimal or something like it. That would be nice :). My only concern is that we may end up locking this PR now, never refactor it with a promise to solve the problem with a more elegant solution and never get it to production.

  5. development reloads were your main concern
    You're right, they were my main concern but then I saw that we could also deliver production value without too much effort.

  6. not ideal for maintainability
    You're right again. I didn't want to refactor the autoupdate package extracting the common code to keep this PR simple and easy to review and also because I wanted to check how well it would be received before committing more time to it. But yeah, there are some function extractions to be made here.

  7. Client-only refreshes are not working
    This one got me really concerned but I think you might be mixing unrelated things. When you're running a standard --minimal app and change any css file, Meteor recompiles the unified stylesheet for your app, prints 'Meteor server restarted' on your console, and waits for you to hit the refresh button to deliver that new stylesheet. This is the current production behavior.
    What autoupdate-polling does is to poll the server to check if the client has the latest version of that unified stylesheet. If it doesn't, the client will *override only the stylesheet and the browser will remain still. If any javascript code is changed (client's or server's) Meteor will also print 'Meteor server restarted'. This time the whole browser will be refreshed.
    So autoupdate-polling does not propose to be a solution to client-only refreshes.

Did I forget anything? Anyway, thank you again for the comment :)

@klaussner
Copy link
Contributor

Thank you for the detailed response! 🙂

autoupdate-polling will not demand any configuration unless the user wants to change the polling frequency or wants to use it to hot reload production code.

I agree that three seconds are a good trade-off and will probably work for many use cases. However, one of my favorite Meteor features is that it makes configuration not only optional but unnecessary/impossible in many cases. One example of this is code splitting, which often requires some configuration or tweaks in other build tools to make sure that modules aren't downloaded multiple times. In Meteor it just works out of the box and can't even be configured, and in most cases I prefer solutions like that.

Since you had worked with it before, would you be willing to test an adaptation of autoupdate-polling to work with SSE? [...] My only concern is that we may end up locking this PR now, never refactor it with a promise to solve the problem with a more elegant solution and never get it to production.

Agreed! The development experience with the minimal template isn't great at the moment and should be improved soon. I'll try to find some time on the weekend or next week to look into it.

This one got me really concerned but I think you might be mixing unrelated things.

Sorry, my comment was a little vague. What I meant was that the server/build tool doesn't support client-only refreshes with autoupdate-polling. If only client-side resources are changed, Meteor can avoid a server restart and only reload the client resources (on the server), which can be significantly faster. In this case, "Client refreshed" is printed instead of "Server restarted." The reason why this isn't working is that Meteor checks if a package named autoupdate is installed before reloading client resources. We could solve this by making the check more lenient, e.g., by looking for package names that start with autoupdate instead of for an exact match.

@filipenevola filipenevola added the in-discussion We are still discussing how to solve or implement it label Oct 28, 2020
@filipenevola filipenevola removed this from the Release vNext milestone Feb 25, 2021
@filipenevola
Copy link
Collaborator

Hi, as we are not having many discussions around this PR I'm closing it.

Of course, if we have new ideas we can re-open it. No problem.

@filipenevola filipenevola removed the in-discussion We are still discussing how to solve or implement it label Mar 3, 2021
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

5 participants