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

Implemented async dynamic attributes (closes #443). #460

Merged
merged 3 commits into from Feb 29, 2024

Conversation

radekmie
Copy link
Collaborator

@radekmie radekmie commented Jan 13, 2024

In this pull request, I added support for async dynamic attributes, i.e., <img {{getAttributesAsync}}>. As usual, while the Promise is still pending or rejected, an empty state is rendered, i.e., with no attributes at all.

EDIT 2024-01-24: As in this discussion, I also added error handling to all async-related logic. It's based on the Blaze._reportException helper.

@radekmie radekmie added this to the Blaze 3.0 milestone Jan 13, 2024
@radekmie radekmie linked an issue Jan 13, 2024 that may be closed by this pull request
packages/blaze/materializer.js Outdated Show resolved Hide resolved
packages/blaze/materializer.js Outdated Show resolved Hide resolved
@harryadel
Copy link
Contributor

What's holding this PR from merging?

@jankapunkt
Copy link
Collaborator

@harryadel as I mentioned in the discussion above, the Promises should be catched and errors piped into the internal error handling, in order to make effective use of APM etc.

@jankapunkt
Copy link
Collaborator

Looks good from my end!

@Grubba27
Copy link
Contributor

Grubba27 commented Jan 25, 2024

Should this become a new Release for blaze? or are we waiting for other PRs to make a pr with all these changes?

Like, this and #458 together ?

@radekmie
Copy link
Collaborator Author

I think it'd be best to release it soon, though I'm not sure what's the state of other PRs. @jankapunkt?

@Grubba27
Copy link
Contributor

this will be a minor correct?

@radekmie
Copy link
Collaborator Author

Yes, a minor - there's new functionality here.

@jankapunkt
Copy link
Collaborator

I would only add the tests, defined in #458 to make sure this PR also fixes #456

@radekmie
Copy link
Collaborator Author

I thought we agreed to add tests in your #458?

@jankapunkt
Copy link
Collaborator

Yes, it's targeting this branch as merge target

@dengribar
Copy link

Hi guys! Is this PR going to be merged soon? Are there any blockers?

@radekmie
Copy link
Collaborator Author

I was certain it was already merged and released... @Grubba27, could you take a look?

@Grubba27
Copy link
Contributor

I was certain as well. Let me check that.

@Grubba27
Copy link
Contributor

Well for some reason this got lost 😢

I thought this PR and this #428 were the same. I will merge and make a new minor, v2.9(I'm not sure because this should have come with 2.8; maybe this should be a 2.8.1?)

Also, is there any other PR that should come along this one @radekmie @jankapunkt ?

@radekmie
Copy link
Collaborator Author

I will merge and make a new minor, v2.9(I'm not sure because this should have come with 2.8; maybe this should be a 2.8.1?)

No, this is a new feature - it should be 2.9.

Also, is there any other PR that should come along this one @radekmie @jankapunkt ?

As mentioned above, this one is first, then #458.

@Grubba27 Grubba27 changed the base branch from master to release-2.9 February 29, 2024 13:23
@Grubba27 Grubba27 mentioned this pull request Feb 29, 2024
@Grubba27 Grubba27 merged commit 4297bab into release-2.9 Feb 29, 2024
2 checks passed
@Grubba27
Copy link
Contributor

I've created the #463 PR

@radekmie radekmie deleted the async-dynamic-attributes branch February 29, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Blaze development
Awaiting triage
Development

Successfully merging this pull request may close these issues.

Support for async dynamic attributes
5 participants