Be SSR friendly when accessing DOM objects

[Johann-S]

Rebased version of: #33131

TODO:

• make sure we have replaced all instances of document and window
• verify ESLint overrides

[xvaara]

Fixed a few omissions in SSR when testing stuff. Can't make a pull request to pull request.

xvaara@835f669

[XhmikosR]

@xvaara you should be able to open a PR against this branch. If you still can't I'd need to cherry pick it, but makes me wonder how those weren't caught.

[xvaara]

#35165 has my changes.

[XhmikosR]

@GeoSot better not rebase it when there are other PRs open against this branch... Anyway, I'll fix the conflicts in #35165, but I'm still not sure about our approach. I mean, how come ESLint didn't catch the issues from #35165?

[xvaara]

Added the last missing in #35339

[xvaara]

I added back the check for addEventListener in my pull request since I received no input about it.
@GeoSot @Johann-S I noticed that this branch is in "Reviewer approved" in project 5.2. Can I ask how you test ssr, since this branch isn't working in ssr?

[GeoSot]

@xvaara as I have seen too, the checks are done by eslint-plugin-ssr-friendly, which theoretically cares about these rules.

Can you propose any other way, to help make the checks more strict?
@Johann-S any opinions on this?

[xvaara]

I'm using Vite and vite-ssr. Some of these problems only came up when I was actually rendering the page.

example: bootstrap-vue-next/bootstrap-vue-next#147

[xvaara]

Just tested with straight running the bundle with node it fails on this branch, but returns without fail on my branch:

# cp dist/js/bootstrap.esm.js dist/js/bootstrap.esm.mjs
# node dist/js/bootstrap.esm.mjs                       
file:///Users/xdanger/tmp/bootstrap/dist/js/bootstrap.esm.mjs:481
  element.addEventListener(typeEvent, fn, delegation);
          ^

TypeError: element.addEventListener is not a function
    at addHandler (file:///Users/xdanger/tmp/bootstrap/dist/js/bootstrap.esm.mjs:481:11)
    at Object.on (file:///Users/xdanger/tmp/bootstrap/dist/js/bootstrap.esm.mjs:514:5)
    at enableDismissTrigger (file:///Users/xdanger/tmp/bootstrap/dist/js/bootstrap.esm.mjs:753:16)
    at file:///Users/xdanger/tmp/bootstrap/dist/js/bootstrap.esm.mjs:842:1
    at ModuleJob.run (node:internal/modules/esm/module_job:185:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:281:24)
    at async loadESM (node:internal/process/esm_loader:88:5)
    at async handleMainPromise (node:internal/modules/run_main:65:12)

Node.js v17.0.1

and the CommonJS bundle fails also:

# node dist/js/bootstrap.bundle.js
/Users/xdanger/tmp/bootstrap/dist/js/bootstrap.bundle.js:485
    element.addEventListener(typeEvent, fn, delegation);
            ^

TypeError: element.addEventListener is not a function
    at addHandler (/Users/xdanger/tmp/bootstrap/dist/js/bootstrap.bundle.js:485:13)
    at Object.on (/Users/xdanger/tmp/bootstrap/dist/js/bootstrap.bundle.js:518:7)
    at enableDismissTrigger (/Users/xdanger/tmp/bootstrap/dist/js/bootstrap.bundle.js:757:18)
    at /Users/xdanger/tmp/bootstrap/dist/js/bootstrap.bundle.js:846:3
    at /Users/xdanger/tmp/bootstrap/dist/js/bootstrap.bundle.js:7:83
    at Object.<anonymous> (/Users/xdanger/tmp/bootstrap/dist/js/bootstrap.bundle.js:10:3)
    at Module._compile (node:internal/modules/cjs/loader:1095:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1147:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)

Node.js v17.0.1

[flippidippi]

Any updates on this? We are using Bootstrap with Next.js and running into these issues.

[GeoSot]

We discussed it with @XhmikosR and concluded that it is not perfect (as it seems linter misses some cases), but it is a nice start to support ssr. So I re-re-based it, wishing for last time

@xvaara I am aware of your PR and it is much appreciated. As the linter doesn't inspect any other errors, we will go with it, and in order to proceed on future PRs we will need some tests, to support the proposed fixes. So if you are aware of any simple way, you are welcome to help on this

[XhmikosR]

1. I still find some instances of document and window in the codebase; shouldn't these change too?
2. I'm not happy with this approach because it doesn't seem to catch everything, but I guess it's better than nothing. That being said, if anyone is up to it, happy to have a smoketest or a dedicated test with its own package.json that we could run on CI only. That way we should be able to catch more issues and perhaps drop the ESLint plugin

[TCB13]

Thank you guys for the efforts around this issue.

I'm using BS5 with Angular's pre-rendering and since it bundles the entire app with Webpack and I can't simply load a module when I need it. I've posted about this here: https://stackoverflow.com/questions/76005487/angular-prerendering-using-js-conditionally-causes-document-is-not-defined

Now, one of the problems I see is that Bootstrap shouldn't assume it is always running on a browser and start calling stuff like document as soon as it is imported/loaded - or at least we could have an option to control when it does it.

[TCB13]

Just an update, I tried the code in this PR and it seems to fix the document related issues, however I now have an element.addEventListener is not a function error.

⠦ Prerendering 5 route(s) to ....\browser...Unhandled Promise rejection: element.addEventListener is not a function ; Zone: <root> ; Task: Promise.then ; Value: TypeError: element.addEventListener is not a function
    at Function.Module._load (internal/modules/cjs/loader.js:790:12) TypeError: element.addEventListener is not a function
✖ Prerendering routes to ... failed.
element.addEventListener is not a function

I traced this back to here

bootstrap/js/src/dom/event-handler.js

Line 201 in 331a935

element.addEventListener(typeEvent, fn, delegation)

After wrapping it like this:

  if (element && typeof element.addEventListener === 'function') {
    element.addEventListener(typeEvent, fn, delegation)
  }

I got Cannot read property 'dir' of undefined. I assume that dir is located here

bootstrap/js/src/util/index.js

Line 235 in 331a935

const isRTL = () => getDocument().documentElement.dir === 'rtl'

I proceeded to change the function to:

const isRTL = () => {
  const docEl = getDocument().documentElement
  if (docEl === undefined) {
    return false
  }

  return docEl.dir === 'rtl'
}

And finally I got a working build that can be used with Angular's pre-rendering!

Can you guys @Johann-S or @XhmikosR apply this changes, or some similar code that you find more reasonable into this PR?

Thank you.

[TCB13]

I did manage to commit the proposed changes in another PR at #38460.

Thank you.

[XhmikosR]

Closing due to being outdated.