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

being ssr friendly when accessing dom objects #33131

Closed
wants to merge 2 commits into from
Closed

Conversation

Johann-S
Copy link
Member

Hi :)

A small PR to allow Bootstrap to be SSR friendly.

I had this idea in mind for quite a long time and it'll avoid developers some headache on how to handle errors like document is undefined or windows is undefined if they use SSR frameworks (for example Gatsby or Next.js)

I added an ESLint plugin to enforce that in all of our source code: https://www.npmjs.com/package/eslint-plugin-ssr-friendly

If you think it's not a relevant do not hesitate to close my PR 👍

@Johann-S Johann-S requested a review from a team as a code owner February 17, 2021 07:46
@XhmikosR
Copy link
Member

Hey, @Johann-S!

TBH I haven't used Bootstrap in such apps. I'd definitely want Bootstrap to work in SSR.

That being said, do we really need the new ESLint plugin? Are there any other cases we might hit in the future?

@Johann-S
Copy link
Member Author

I added this Eslint plugin to be sure we won't forget about that compatibility. But we can hit those kind of problem if we access global browser vars without wrapping them in a function which check if those vars are available or not

@XhmikosR
Copy link
Member

@Johann-S I say we go with it, but I have zero experience with such projects, so as long as this solves the issue it's fine with me.

Do we need to do anything for v4?

@Johann-S
Copy link
Member Author

Johann-S commented Feb 19, 2021

about our v4 it depends if there is a plan to release a v4.7 or not because it's clearly a new feature to me but yes those changes have to be added to v4 too if there is a v4.7 plan

@dakur
Copy link

dakur commented Feb 22, 2021

Related: it's not just about SSR, I've struggled with window and document hard-coded as well when we needed to use scrollspy on content in an iframe. Finally, I've had to make a fork which patches this stuff in a quick-fix way, but fixing it properly would be much appreciated from my side. 🙂

@XhmikosR
Copy link
Member

XhmikosR commented Mar 2, 2021

@Johann-S can you please rebase this one? After we land this, we should backport it

@GeoSot GeoSot force-pushed the jo-ssr-friendly branch 2 times, most recently from a303127 to 19c6be8 Compare March 11, 2021 00:41
@alpadev
Copy link
Contributor

alpadev commented Mar 13, 2021

I'm thinking of a problem with that approach tho I'm not sure if it really applies.

You're mocking document/window with an object but what if our code tries call a function on that? Wouldn't that throw an error as well e.g. getDocument().addEventListener is not a function?

@Johann-S
Copy link
Member Author

@alpadev if the code is ran in a browser env it won't throw an error because window/document are available. It's just for SSR frameworks which pre build website with Node.js to avoid build errors. See : https://www.gatsbyjs.com/docs/debugging-html-builds/#how-to-check-if-window-is-defined

@alpadev
Copy link
Contributor

alpadev commented Mar 13, 2021

Hm yeah but isn't in that case the source ran within an environment e.g. node (that throws that window is undefined error)?

So if that environment evaluates our code it will come accross EventHandler.on() on a module level and tries to execute and as such throws another error?

@alpadev
Copy link
Contributor

alpadev commented Mar 14, 2021

The reason why I'm asking. The last time I stumbled across that, wrapping my code within something like

if (typeof window !== 'undefined') { require(...) }

solved it for me.

@Johann-S
Copy link
Member Author

@alpadev yep if the code is ran in a Node env it'll throw an error but Bootstrap isn't designed to be ran in such env. SSR frameworks doesn't really ran the code in a Node env, they built it that's all

@alpadev
Copy link
Contributor

alpadev commented Mar 14, 2021

@Johann-S I see. Looks like I had a misconception of how things in SSR really work then. I always imagined the code to be executed on the server environment but I'm lacking real world experience with that.

I played around a bit with SSR in the scope of Vue some time ago but never used in production so far and didn't really dig into it that deep. Well then just ignore my concerns.

GeoSot
GeoSot previously approved these changes Mar 15, 2021
@XhmikosR
Copy link
Member

@Johann-S can you please check the changes so far and see if everything still looks good to you?

@alpadev
Copy link
Contributor

alpadev commented Mar 17, 2021

cough 😏

image

Maybe we should review our bundling or how files are refrenced in the package.json. I'm not an expert when it comes to this but I could imagine if we add some static wrapper file that would require and export our bundle only when there is some window. And reference that in the package.json main/module we could achieve the same without changing our code. Just an idea.

@XhmikosR
Copy link
Member

Removed from the beta3 project due to lack of responses.

@mdo
Copy link
Member

mdo commented Jun 14, 2021

Should we revisit this or close as stale?

@XhmikosR
Copy link
Member

Would be nice to have this but yeah right now it's outdated.

@XhmikosR XhmikosR closed this Jun 15, 2021
@XhmikosR XhmikosR deleted the jo-ssr-friendly branch June 15, 2021 18:04
@dakur
Copy link

dakur commented Jun 21, 2021

@mdo @XhmikosR What about using scrollspy in a way that spied elements are inside of iframe while the navigation is outside of it? I have such use-case as I mentioned above and currently with hard-coded window and document it is impossible to do it properly without patching scrollspy package.

Ah, you've created new issue, I didn't see it at first, sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants