perf: reduce query selector

[gajus]

Description

Reduces use of unnecessary querySelector use.

Additional context

It is faster to read DOM once.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[gajus]

Related, though I didn't want to add it to this PR, why is this being awaited in the first place?

I didn't try this, but it should be just:

-  return Promise.all(
-    deps.map((dep) => {
-      // @ts-ignore
-      dep = assetsURL(dep, importerUrl)
-      // @ts-ignore
-      if (dep in seen) return
-      // @ts-ignore
-      seen[dep] = true
-      const isCss = dep.endsWith('.css')
-      // @ts-ignore check if the file is already preloaded by SSR markup
-      if (documentLinkUrls.includes(dep)) {
-        return
-      }
-      // @ts-ignore
-      const link = document.createElement('link')
-      // @ts-ignore
-      link.rel = isCss ? 'stylesheet' : scriptRel
-      if (!isCss) {
-        link.as = 'script'
-        link.crossOrigin = ''
-      }
-      link.href = dep
-      // @ts-ignore
-      document.head.appendChild(link)
-      if (isCss) {
-        return new Promise((res, rej) => {
-          link.addEventListener('load', res)
-          link.addEventListener('error', () =>
-            rej(new Error(`Unable to preload CSS for ${dep}`))
-          )
-        })
-      }
-    })
-  ).then(() => baseModule())
+  for (const dep of deps) {
+    // @ts-ignore
+    dep = assetsURL(dep, importerUrl)
+    // @ts-ignore
+    if (dep in seen) return
+    // @ts-ignore
+    seen[dep] = true
+    const isCss = dep.endsWith('.css')
+    // @ts-ignore check if the file is already preloaded by SSR markup
+    if (documentLinkUrls.includes(dep)) {
+      return
+    }
+    // @ts-ignore
+    const link = document.createElement('link')
+    // @ts-ignore
+    link.rel = isCss ? 'stylesheet' : scriptRel
+    if (!isCss) {
+      link.as = 'script'
+      link.crossOrigin = ''
+    }
+    link.href = dep
+    // @ts-ignore
+    document.head.appendChild(link)
+  }
+
+  return baseModule();

Waiting for scripts to be actually loaded is not needed for any logic that follows, as far as I can tell.

[gajus]

Please re-run tests. It looks like it is a CI/CD failure.

[gajus]

@1yyx990803 @patak-dev can someone re-run tests?

[tony19]

@gajus A couple notes about your proposed code below:

for (const dep of deps) {
  // @ts-ignore
  dep = assetsURL(dep, importerUrl)
  // @ts-ignore
  if (dep in seen) return
  // @ts-ignore
  seen[dep] = true
  const isCss = dep.endsWith('.css')
  // @ts-ignore check if the file is already preloaded by SSR markup
  if (documentLinkUrls.includes(dep)) {
    return
  }
  // @ts-ignore
  const link = document.createElement('link')
  // @ts-ignore
  link.rel = isCss ? 'stylesheet' : scriptRel
  if (!isCss) {
    link.as = 'script'
    link.crossOrigin = ''
  }
  link.href = dep
  // @ts-ignore
  document.head.appendChild(link)
}

return baseModule();

• Only the first @ts-ignore is needed to silence the error for assetsURL, which is dynamically generated.
• const dep of deps should be let dep of deps, since dep is re-assigned.

[gajus]

@gajus A couple notes about your proposed code below:

for (const dep of deps) {
  // @ts-ignore
  dep = assetsURL(dep, importerUrl)
  // @ts-ignore
  if (dep in seen) return
  // @ts-ignore
  seen[dep] = true
  const isCss = dep.endsWith('.css')
  // @ts-ignore check if the file is already preloaded by SSR markup
  if (documentLinkUrls.includes(dep)) {
    return
  }
  // @ts-ignore
  const link = document.createElement('link')
  // @ts-ignore
  link.rel = isCss ? 'stylesheet' : scriptRel
  if (!isCss) {
    link.as = 'script'
    link.crossOrigin = ''
  }
  link.href = dep
  // @ts-ignore
  document.head.appendChild(link)
}

return baseModule();

• Only the first @ts-ignore is needed to silence the error for assetsURL, which is dynamically generated.
• const dep of deps should be let dep of deps, since dep is re-assigned.

None of the code that you reference is added by this PR.

[tony19]

Yeah, I'm aware 😄 Otherwise, I would've noted it in a PR review. I was merely commenting.

[bluwy]

This now skips checking [rel="stylesheet"] if isCss is true, which would regress #1734. We should probably check the href and rel="stylsheet" too for css.

[bluwy]

Related, though I didn't want to add it to this PR, why is this being awaited in the first place?

It's likely to prevent FOUC if the preload links aren't properly loaded yet, and also to prevent runtime race conditions between preload deps and base module.

[bluwy]

Closing as this PR brings in breaking changes.