Use browser field in package.json instead of main field

[oroce]

We've been using picturefill for like 4months, we are absolutely satisfied with it but as we are moving our React rendering to the backend we realized picturefill breaks in nodejs. Since it uses main field in package.json node tries to load the dist/picturefill.js which contains window.matchMedia and window in node is undefined.

Solutions:

1. Wrap polyfill in if where we are checking for the existance of window

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

   Imho it looks ugly:(

2. Use matchmedia npm module which works on backend and client side as well (we should add a build step to picturefill)

3. Use browser field instead of main field

I would go with the 3rd one. Here why:
polyfill shouldn't work on backend since it uses matchMedia and picturefill shouldn't support node, but it does not mean it should break.
Browser field is a de facto way of separating client side module from backend, here's the spec: https://gist.github.com/defunctzombie/4339901

Also the 3rd way wont break any use cases:

• if you are using plain old <script> tag, you would still download the dist package
• if you are using browserify, it does support browser field: https://github.com/substack/node-browserify#browser-field
• if you are using webpack, it does support browser field: http://webpack.github.io/docs/configuration.html#resolve-packagemains

I moved dist/picturefill.js to browser field and changed the main field empty.js, which just an empty js file.

[mike-engel]

Just curious @oroce, are you using picturefill on the backend for server-side rendering, or something else?

[oroce]

Not directly. We are using a react component (what we are rendering on server side) which does basically the following thing:

import picturefill from 'picturefill';
MyPictureComponent extends React.Component {
   componentDidUpdate() {
     picturefill();
  }
}

window.matchMedia line from picturefill gets executed when the module gets compiled. (in line 1).

[mike-engel]

Ok, this makes sense to me, but I'm trying to think of situations where moving it to browser would be detrimental, if that situation exists. Thoughts from @aFarkas and @baloneysandwiches?

[aFarkas]

I don't have any idea how this is actually treated, but if you add empty.js than there is no function (not even a noop) returned. So doesn't this produce an error?

picturefill is not a function/picturefill is undefined

How do other libs treat this problem? Pretty sure picturefill isn't the only script.

[mike-engel]

I know npm will error out if it can't find the file in the main field, but I'm not sure it errors if it can't find a module.exports. @oroce I'm assuming you've tested this and found that there was no error?

[oroce]

@mike-engel Yes, require will throw an error if a file is not found.

@aFarkas yes it wont be a function.

There are two options:

1. Export nothing and it'll be a breaking change aka 3.0.0
2. Export an emty function which will be a minor change 2.4.0

I can live with any of them

[ruffle1986]

Yes, but to go with the 1. option, don't we have to export a noop function to avoid getting uncaught errors as @aFarkas mentioned?

[oroce]

haha just realized, if you want to use both in node and in browser you can do:

var picturefill;
try {
  picturefill = require('picturefill');
} catch(x) {}

if (picturefill == null) {
  // we are in node
}
else {
  picturefill(); // browser
}

[mike-engel]

Sorry for the long delay @oroce. Are you comfortable with your try/catch method, or would you rather see this PR go through? If the PR does go through, I'd like to see PF still exported via main, but print a deprecation warning (since this would now have to go in 4.0). Thoughts?

[oroce]

@mike-engel I would be happy with wrapping require in try/catch but unfortunately ES6 module spec does not allow having import, export placed anywhere than top level.

All flavors of import and export are allowed only at toplevel in a module. There are no conditional imports or exports, and you can’t use import in function scope. (src)

Shall I update the pull request?

[jegtnes]

Just looking at this. So from what I understand, if there is a simultaneous browser field and main field, Browserify/Webpack/et al use browser instead main when in a browser environment, but won't ignore main if there's a browser, and this is where it throws errors in windowless JS environments. That makes sense, but is slightly problematic for us. This might be a breaking change depending on your setup (unless all module bundlers have had support for the "browser" field for forever, but it seems like a new-ish thing, so it could break someone's setup, if they aren't zealous with their version updates). So, ideally, this would go into a new major version, while baking in a deprecation notice for 3.x, like @mike-engel said.

What's the best way to approach this, then?

[oroce]

@jegtnes the first part is right.

Actually browser field is a kinda old thing. In browserify it was introduced back in 2013. (browserify/browserify#174)

Webpack uses enhanced-resolve which does support browser field, and I really believe browser field has become a de facto standard.

[mike-engel]

Ok @oroce, I still think this is a good idea. We want to wait, however, until we're ready to release a Picturefill 4.0 since this would definitely be a breaking change. I'm not too concerned yet since import isn't natively supported anywhere yet, and even with webpack and rollup's tree shaking, little, if anything, would be removed from the pf import. Until then, I think the try/catch or conditional loading tricks will work. Does that sound ok to you?

[oroce]

@mike-engel yup, I agree that we should wait until 4.0. In 3.X the recommended way should be using try/catch or conditional loading.

[chemoish]

+1

[fregante]

This doesn't seem right, you can't expect all libraries to use the browser field just because they use window. You'd probably be better off using something like jsdom on node (or just the try/catch) because inevitably you'll want some browser module to run on the server but they have a blanked-out main field.

[scottjehl]

Thank you for all of the thoughts on this. As I'm archiving and deprecating this project per #723 I'm going to close this without implementing a big change. Thanks!