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

Feature mongo async #16

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Feature mongo async #16

wants to merge 15 commits into from

Conversation

Grubba27
Copy link
Contributor

@Grubba27 Grubba27 commented Sep 6, 2022

This PR is about updating the tutorial to start using the mongo API introduced in Meteor 2.8

Copy link
Member

@henriquealbert henriquealbert left a comment

Choose a reason for hiding this comment

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

Make sure to test with Cordova on iOS and Android. If all tests are okay, we are good with this one :)
Don't forget to bump the Meteor version too

tutorial/changelog.md Outdated Show resolved Hide resolved
@Grubba27
Copy link
Contributor Author

Grubba27 commented Sep 8, 2022

Awesome! when next version is out I will bump & merge :)

@@ -22,22 +21,19 @@
const pendingOnlyFilter = { ...hideCompletedFilter, ...userFilter };


tasks = user
getTasks = user
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if it is better to use the sync Mongo api in Svelte components. I am planning to do that in my projects. The async api adds a lot of complexity here, but doesn't seem to have any benefits. Meteor will not be able to remove the sync Mongo api from the client for a long time if ever since there are too many places it is used that only support sync code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I thought a bit about this, but I also liked using promises with Svelte. Guess I could show that it can be used both ways. Using promises feels like when we are creating UI in other places (Svelte /SvelteKit).
Don't know what to do when the release is launched. What is your opinion @zodern?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The async api seems unnecessary since the client isn't waiting for anything. It works the same as the sync api, but you now have to wait for the promise to resolve.

It isn't uncommon to run a query, and then use the results within the same or another reactive/tracker statement. With the async api it isn't easy to do. This is a larger problem because you can't use Tracker.autorun in all of the places svelte's normal dependency tracking works, so how you would normally handle this in svelte might not work with Meteor. I'm looking into how we can allow using Tracker.autorun with @const tags, which will partially help with this.

Another minor thing is using #await encourages creating a loading state, but it will never be shown since the promise should resolve before Svelte updates the DOM. If I am wrong and the loading state is briefly shown each time the promise is re-created then that could be a performance and UX problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I looked into it a little more, and it seems every time the promise is re-created, svelte removes all the DOM from the {:then} block, creates the DOM for the loading state, and then in the next micro task removes the DOM for the loading state and recreates the {:then} block.

I created an example in the svelte REPL to show this: https://svelte.dev/repl/c398061483f247b9a77bf767c6d3ab40?version=3.50.1
To make it easier to see what is happening, I added a fade transition to the loading and {:then} blocks.

Without a transition, I'm not able to find a way for the loading state to ever be visible. The browser should run all micro tasks before updating the UI, so it should be impossible for it to ever be visible.

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've seen it now. Not sure what to do. Perhaps In the tutorial use?

{#await promise then value}
	<p>the value is {value}</p>
{/await}

At some point, those Database methods will be async (if I understood correctly here).
what would you say is the best way to handle this matter then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would be a less verbose way of doing it. Another option is to do something like:

let tasks = [];
$m: {
  Tasks.find().fetchAsync().then(value => tasks = value);
}

This would be significantly more efficient compared to #await when the data frequently changes, and allow you to use the data in other reactive or tracker statements.

At some point, those Database methods will be async (if I understood correctly meteor/meteor#12028).
what would you say is the best way to handle this matter then?

The sync api will stop working on the server when Fibers are unavailable, but the plan is to keep it on the client, possibly forever, since there are too many places that can only use the sync api including Blaze helpers and Tracker.autorun.

Comment on lines +30 to +39
getTasks = user
? TasksCollection.find(
hideCompleted ? pendingOnlyFilter : userFilter,
{ sort: { createdAt: -1 } }
).fetchAsync()
: [];

incompleteCount = TasksCollection.find(pendingOnlyFilter).count();

pendingTasksTitle = `${
incompleteCount ? ` (${incompleteCount})` : ''
}`;
getCount = user
? TasksCollection.find(pendingOnlyFilter).countAsync()
: 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

These two ternaries are unnecessary since it checked if there is a user on line 22

@@ -62,13 +64,15 @@ Now comes the fun part, you will render the tasks saved in our database. With Sv

On your file `App.svelte`, import the `TasksCollection` file and, instead of returning a static array, return the tasks saved in the database. Let's use an extension of the Svelte's [$ reactive statements](https://svelte.dev/docs#3_$_marks_a_statement_as_reactive) feature, to maintain your tasks, called [$m](https://github.com/zodern/melte#tracker-statements):

Because it is an asynchronous function, we need to use the `#await` keyword to wait for the data to be fetched from the database. More information about how Svelte handles asynchronous values can be found [here](https://svelte.dev/docs#template-syntax-await).
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is easy to misunderstand this as meaning it waits for the data to be fetched by the server. Instead, fetchAsync will almost immediately resolve with whatever data the client currently has, and when the data changes a new promise will be created that resolves with the new data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this paragraph is misleading? I thought that this could be a good way to show how to handle these cases

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is a good explanation. It maybe just needs to clarify what is meant by wait and the database. When I first read it, I thought it was saying it was waiting for the complete set of data to be retrieved from the server. Part of the confusion might also be from that is what promises are usually used for, and what the promise is for here is different (fetchAsync isn't waiting for anything to happen. The only reason it returns a promise is to be consistent with the server).

@@ -111,7 +111,8 @@ All that is left now is to make one final change: we need to show the newest tas
```html
<script>
..
$m: tasks = TasksCollection.find({}, { sort: { createdAt: -1 } }).fetch()
let getTasks;
Copy link
Collaborator

@zodern zodern Sep 9, 2022

Choose a reason for hiding this comment

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

This line isn't needed. Svelte (or in this case, zodern:melte since it is a tracker statement) will inject the variable declaration for you when compiling the file (https://svelte.dev/docs#component-format-script-3-$-marks-a-statement-as-reactive)

$set: { isChecked: !task.isChecked }
});
const toggleChecked = async () => {
// Set the checked property to the opposite of its current value
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be isChecked instead of checked

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.

3 participants