Handle popup right after first popup detected (#326)

[seia-soto]

This PR adds immediate processing of first appeared popup to fix #326 . Instead of waiting all promises, I added a callback to handle first appearing popup.

// Function signature
detectPopups(cmps: AutoCMP[], onFirstPopupAppears: (cmp: AutoCMP) => unknown = () => {}): Promise<AutoCMP[]>

By using callback, the popup code won't be disturbed by other CMP detection codes. Also, I could maintain the code consistency.

Code summarization:

async _start() {
	// Detect CMPs

	let result: Promise<boolean>
	
	// Adds first popup callback (which is handled by detectPopups
	let foundPopups = await this.detectPopups(staticCmps, cmp => { result = this.handlePopup(cmp) })
    if (foundPopups.length === 0) {
      foundPopups = await this.detectPopups(cosmeticCmps, cmp => { result = this.handlePopup(cmp) })
    }

	if (foundPopups.length === 0) {
      // No popup found
    }

    if (foundPopups.length > 1) {
      // Multiple popup found
    }

    return result
}

This PR also includes the following changes:

• Change Array.prototype.map iteration to single loop to filter out CMPs including cosmetic rules (performance reasons)
• Adds test-results/ and .DS_Store to .gitignore

Some suggestions:

• Add stricter eslint config for code style consistency
• Add contributing.md file for future contributors

refs #326
refs #319

[seia-soto]

I'd like to accept every code style fixes. Please, feel free to leave a comment.

[muodov]

Thanks @seia-soto, I really appreciate your help! This looks great to me, but I'd like @sammacbeth to have a second look since this is a core change.
Note that this is a holiday season, so a turnaround may be a bit slow 🙏

[sammacbeth]

Could this be rewritten with async/await? I find that easier to follow, and matches the code-style in the rest of the project.

[seia-soto]

I converted the function using Promise.race and Promise.allSettled. Generally agreeing to code-style problem.

[seia-soto]

• changing as later review stated that Promise.any should be used.

[sammacbeth]

I'm holding off on merging here, as I noticed an increase in test failures when running the Playwright tests from #323 on this branch. I need to investigate if this might have caused a regression.

[seia-soto]

@sammacbeth @muodov Sorry for making confusion. I found that autoconsent may return unassigned result variable before the callback function of onFirstPopupAppears because Promise of detectPopups can be resolved before the Promise of handlePopup. I pushed a fix to this just now. Can you check again?

• edit 1: However, I don't think this will happen in common case considering computation times after result assignment.

[sammacbeth]

@seia-soto Can you rebase with main? Then I'll run a new round of tests to check if this fix is working correctly.

[seia-soto]

@sammacbeth done!

[sammacbeth]

Thanks, it looks like there was an issue with the rebase because your diff is now much too large. I created a clean branch and cherry-picked your commits to test: https://github.com/duckduckgo/autoconsent/tree/sam/327-rebase

My testing found that this change is failing in quite a few cases with this error:

For example, try the following sites:

• https://www.esrgear.de/
• https://starthardware.org/
• https://www.codeproject.com/

[seia-soto]

@sammacbeth Hi sam, it seems like we can simply put error handler there. Can you rerun your test on the following branch?

https://github.com/seia-soto/autoconsent/tree/sam/327-rebase

[sammacbeth]

Thanks @seia-soto. This looks to work better. Can you open a new PR based on that branch (or fix the merge and conflicts on this PR), then I'll give this another pass.

[seia-soto]

@muodov Please, take a look into the result of merge so your recent feature push can work properly :)

• 1636c27 (#327)

Maybe it's better to see total diff: https://github.com/duckduckgo/autoconsent/pull/327/files

[muodov]

@seia-soto the merge update looks good to me

[seia-soto]

@sammacbeth I'm now ready to merge!

[muodov]

FYI #365