search

cee-chen / object-fit-polyfill

A Javascript polyfill for browsers that don't support the object-fit CSS property.
ISC License
496 stars 93 forks source link

return if the page is being rendered on the server #31

Closed Trevomatic closed 7 years ago

Trevomatic commented 7 years ago

This is the only polyfill I could find that seems to work properly with videos on IE and Edge, so than you so much for creating this @constancecchen!

I am trying to use this polyfill with a React app, and this breaks when doing server side rendering and document is not defined.

I just check to see if window is undefined and return, which allows us to import objectFitPolyfill at the top of the file, and not have to require it in componentDidMount. Please let me know if you think this a good approach to handle this situation.

cc @Eschon

saponifi3d commented 7 years ago

Alternatively, you can export browser in package.json to be the current main.

Then you can export main to be a server variant of the file.

this only works for webpack / browserify though... but would also make sense because build systems would generally need this to require / import the file through main anyways.

Eschon commented 7 years ago

Looks good to me, I can test it next week at work.

cee-chen commented 7 years ago

I don't have enough familiarity with React to say if this is the best solution for their ecosystem/framework - IIRC client-side JS like this polyfill should be going into componentDidMount or another similar lifecycle. However, this is a small enough workaround that I'm fine with it going in.

If I get the opportunity to work with React sometime soon I'll try to figure out what the best practice there is! :)

Eschon commented 7 years ago

I agree that code specific for client-side should go into componentDidMount or similar lifecycle methods. But I still want to import the module on top with all my other imports. The problem with modules like this is that they run as soon as they are imported. To be best suited for server-side rendering like this it would be better to just do the necessary setup and export a function that needs to be called.

But I think this workaround is good enough

saponifi3d commented 7 years ago

@Eschon +1, we should probably just do the package.json changes for main / browser. that would fix the polyfill for importing - then server it just wouldn't import anything - or we could have it export a 'default'ed styling.