FredKSchott / snowpack

ESM-powered frontend build tool. Instant, lightweight, unbundled development. ✌️
https://www.snowpack.dev
MIT License
19.48k stars 922 forks source link

[BUG] DatePicker component doesn't work with Snowpack #3295

Open Dawnthorn opened 3 years ago

Dawnthorn commented 3 years ago

Bug Report Quick Checklist

Describe the bug

When trying to use the DatePicker component from react-datepicker, I get this error:

Unhandled Runtime Error
TypeError: callBind is not a function
Source
http://localhost:8080/_snowpack/pkg/object-is.v1.1.5.js [:41:26]
@http://localhost:8080/_snowpack/pkg/object-is.v1.1.5.js:41:26

To Reproduce

  1. npx create-snowpack-app react-snowpack --template @snowpack/app-template-minimal
  2. cd react-snowpack
  3. yarn add react react-dom react-datepicker
  4. rm index.js
  5. Create index.jsx with this content:
    
    import React from 'react';
    import ReactDOM from 'react-dom';
    import DatePicker from 'react-datepicker';
    import "react-datepicker/dist/react-datepicker.css";

ReactDOM.render(

, document.getElementById('root') ); ``` 5. `yarn start` 6. You should get the error in the browser window that opens. ### Expected behavior I expect to see the date picker which looks like a text input box and when you click on it you get a calendar to choose the date.
Nivisnum commented 3 years ago

Hi,

I've a similar problem and I've been digging in the codebase to find a potential cause. At start I'd like to point out that modularity in JS is magic for me - I might have got it wrong and would love a confirmation/pointers from someone that have better understanding of this topic.

With that out of the way - from what I've found the call-bind lib (and some others) provides a CJS module that have default as well as other named exports. The @rollup/plugin-commonjs configuration for local dependencies (when the snowpack initially transforms them to esm modules) uses the option requireReturnsDefault: 'auto', which is described as:

"auto": This is complementary to how output.exports: "auto" works in Rollup: If a module has a default export and no named exports, requiring that module returns the default export. In all other cases, the namespace is returned. For external dependencies when using esmExternals: true, a corresponding interop helper is added:

// output
import * as dep$1 from 'dep';

function getDefaultExportFromNamespaceIfNotNamed(n) {
  return n && Object.prototype.hasOwnProperty.call(n, 'default') && Object.keys(n).length === 1 ? n['default'] : n;
}

var dep = getDefaultExportFromNamespaceIfNotNamed(dep$1);

console.log(dep);

Sadly, there are more than a single property on our object - at least the default export and the apply. With that the whole namespace will be returned but the dependency that uses call-bind (object-is in this case) tries to use it as if the getDefaultExportFromNamespaceIfNotNamed returned the default export which is a function - and it throws exception here.

From the description of @rollup/plugin-commonjs options it seems like requireReturnsDefault: "preffered" would work better in case of call-bind

"preferred": If a module has a default export, requiring that module always returns the default export, no matter whether additional named exports exist. This is similar to how previous versions of this plugin worked. Again for external dependencies when using esmExternals: true, an interop helper is added:

function getDefaultExportFromNamespaceIfPresent(n) {
return n && Object.prototype.hasOwnProperty.call(n, 'default') ? n['default'] : n;
}

I doubt that changing this option in plugin config would fix everything, as other dependencies might fail. One solution (probably) would be to allow the users to specify different strategy for each dependency passing function to requireReturnsDefault:

To change this for individual modules, you can supply a function for requireReturnsDefault instead. This function will then be called once for each required ES module or external dependency with the corresponding id and allows you to return different values for different modules.

This would have to be exposed by the snowpack config in some way (or maybe it already is?). I will try to confirm this locally but would like for someone from maintainers/contributors to review if this idea is actually valid.

Ps: I was able to run project with similar problem by switching to streaming imports - the object-is sources look different than the output from the @rollup/plugin-commonjs and correctly use the call-bind without additional interop helper. If you don't have dependencies that are not available on skypack you could try the switch - maybe that will help as a workaround.

Ps2: Other discussions about this problem: https://github.com/snowpackjs/snowpack/discussions/2820 https://github.com/snowpackjs/snowpack/discussions/3011

Dawnthorn commented 3 years ago

I looked into this some more too, and setting requireReturnsDefault to preferred in esinstall/src/index.ts fixes the problem for this project. That option is not exposed in the snowpack config right now.

Another way to fix this in snowpack is to have isPackageCJS in snowpack/src/sources/local.ts return true for the call-bind module so that the externalEsm function that gets passed to the rollup esmExternals option returns false in which case it doesn't do the getDefaultExportFromNamespaceIfPresent conversion.

At this point, I'm just not sure what the best way to fix this would be. I don't really understand a lot of the subtleties of dealing with CommonJS and ESM transpiling. Maybe the best thing would just be a way to add a config option for the names of modules to force isPackageCJS to be true so that esmExternals returns false for that module.

drwpow commented 3 years ago

So I’m not sure when this was fixed, either in Snowpack or one of the listed dependencies, but I believe this seems to work now with the most recent versions of everything (Snowpack, react-datepicker, etc.).

Is there a particular version being tested I can look more into?

Dawnthorn commented 3 years ago

Yeah. When I filed the bug the latest version of react-datepicker was 3.8.0 and snowpack 3.7.0 still does not work with that version. Snowpack 3.7.0 is working with the new latest version of react-datepicker which is 4.1.1.

It looks like it's fixed because there's no longer any call-bind dependency and that's the the module with the weird exports.

drwpow commented 3 years ago

Ah thanks. Will look into 3.8.0 just to test a real-world package with call-bind as other packages may have this problem.

rossng commented 3 years ago

I've run into this problem too. The assert package (the browser version of the Node module) depends on object-is, which in turn depends on call-bind.

afeinman commented 3 years ago

I am still running into this problem even with v3.8.1 (see also https://github.com/snowpackjs/snowpack/discussions/3011 ). I don't use react-datepicker, but something else is triggering the object-is / call-bind dependency.

gpascale commented 3 years ago

I looked into this issue independently and believe that @Nivisnum is right on the money with their analysis, however I also believe that changing the value of requireReturnsDefault from 'auto' to 'preferred' is the proper fix because it would restore behavior that I don't think was meant to be changed.

The requireReturnsDefault parameter was introduced in @rollup/plugin-commonjs with version 16.0.0. snowpack changed from 15.0.0 to 16.0.0, as far as I can tell, with version 3.1.0, which is why people experiencing this issue have found a workaround in sticking with 3.0.13. This parameter added some complexity to what the plugin returns when resolving a commonjs module that has a default export. In short, the old behavior would always return the default export if it exists - the new parameter allows you to choose between that or additionally ensuring that there are no exports in addition to 'default'. In 16.0.0, this stricter check is called 'auto', which the old behavior is 'preferred'

Here are the relevant bits in @rollup/plugin-commonjs

@rollup/plugin-commonjs@16.0.0 with requireReturnsDefault: 'auto' (used by snowpack 3.1.0 and later)

function getDefaultExportFromNamespaceIfNotNamed(n) {
  return n && Object.prototype.hasOwnProperty.call(n, 'default') && Object.keys(n).length === 1 ? n['default'] : n;
}

@rollup/plugin-commonjs 15.0.0 (used by snowpack 3.0.13 and before)

export function getCjsExportFromNamespace (n) {
  return n && n['default'] || n;
}

@rollup/plugin-commonjs@16.0.0 with requireReturnsDefault: 'preferred' (same behavior as 15.0.0)

function getDefaultExportFromNamespaceIfPresent (n) {
  return n && Object.prototype.hasOwnProperty.call(n, 'default') ? n['default'] : n;
}

So by setting requireReturnsDefault to 'preferred', the resolution behavior will change back to what it was in snowpack in version 3.0.13 and before. I have confirmed this one line change fixes the object-is/call-bind issue for the current version of snowpack.

Now, my hunch is that this change in behavior was an unintended consequence of upgrading @rollup/plugin-commonjs from 15.0.0 to 16.0.0, and therefore setting requireReturnsDefault to 'preferred' would restore the original and intended behavior. It seems likely that requireReturnsDefault:'auto' was chosen because it seems like a sensible default and without the intention of changing this behavior. If I am wrong about that, or if there are indeed cases which are fixed by this change in behavior, the only real fix is probably to expose requireReturnsDefault somehow on a per-module basis.