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

Enhance reliability of how Site Health enqueued assets module detects assets #135

Closed
felixarntz opened this issue Jan 28, 2022 · 6 comments · Fixed by #136
Closed

Enhance reliability of how Site Health enqueued assets module detects assets #135

felixarntz opened this issue Jan 28, 2022 · 6 comments · Fixed by #136
Assignees
Labels
[Type] Enhancement A suggestion for improvement of an existing feature
Milestone

Comments

@felixarntz
Copy link
Member

The initial implementation for the Site Health enqueued assets module (see #25) is a solid start, but there's room for enhancement in how reliably it detects enqueued assets. This was already pointed out in the initial PR review, but it was decided to merge the large first PR first and then implement this enhancement separately, to allow for a more focused review.

This issue is primarily to address the following review comment: #54 (comment)

There are potentially more aspects we can improve around this, let's explore this here.

@felixarntz felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature [Focus] Site Health Needs Discussion Anything that needs a discussion/agreement labels Jan 28, 2022
@felixarntz
Copy link
Member Author

@aristath Anything to add? Do you think this is already good to open a PR for addressing your comment, or should we evaluate the technical approach a bit more before writing code?

cc @manuelRod

@aristath
Copy link
Member

Assigning this one to myself so I can take a look again on Monday.
I think the code posted in #54 (comment) will be a good starting point, I'll just need to polish and properly test it before submitting a PR 👍

@aristath aristath self-assigned this Jan 28, 2022
@eclarke1 eclarke1 removed the Needs Discussion Anything that needs a discussion/agreement label Jan 28, 2022
@manuelRod
Copy link
Contributor

Hello, @aristath @felixarntz I was working already on the proposed enhances, I didn't realise you have merged the previous PR :D

Anyways, here is the new PR with a few of the things addressed, we can keep the conversation going on there!

#136

@felixarntz
Copy link
Member Author

@manuelRod Oh, sorry about that - I guess I merged that a bit too quickly. But that's great, will review soon!

@eclarke1 eclarke1 added this to the 1.0.0-beta.1 milestone Jan 31, 2022
@manuelRod
Copy link
Contributor

@aristath can you review this PR too?

@felixarntz
Copy link
Member Author

Fixed via #136

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants