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

ES6 Promise instropection not correctly working #178

Closed
facundocabrera opened this issue Mar 3, 2016 · 10 comments
Closed

ES6 Promise instropection not correctly working #178

facundocabrera opened this issue Mar 3, 2016 · 10 comments

Comments

@facundocabrera
Copy link

The following code evaluates to false in Chrome: Version 49.0.2623.75 (64-bit)

var USE_NATIVE = !!function(){
  try {
    // correct subclassing with @@species support
    var promise     = $Promise.resolve(1)
      , FakePromise = (promise.constructor = {})[require('./_wks')('species')] = function(exec){ exec(empty, empty); };
    // unhandled rejections tracking support, NodeJS Promise without it fails @@species test
    return (isNode || typeof PromiseRejectionEvent == 'function') && promise.then(empty) instanceof FakePromise;
  } catch(e){ /* empty */ }
}();

I'm not an expert, but what I can said is, if the environment provides the fetch API, we should consider promises available, because any call to fetch() will return a "native" instance instead of a an instance using the polyfill.

Make sense?

@loganfsmyth
Copy link
Contributor

@dendril Having fetch and having a Promise constructor does not mean that the Promise implementation is spec compliant. The goal of a polyfill is to ensure spec compliance across the board. I believe it's Chrome 50 or 51 that has the first fully compatible implementation.

@facundocabrera
Copy link
Author

I completely agree with you, but doesn't make sense to have a fetch API returning something different from what I can instantiate using a call to the Promise constructor.

@zloirock
Copy link
Owner

zloirock commented Mar 4, 2016

What can I say... If native promise implementation is incorrect - it should be replaced. Most alternative Promise polyfills do the same. Someone just replaces global Promise, for example, to Bluebird. So anyway instanceof detection for most cases makes no sense. In your code you can just wrap fetch and have expected instance:

var _fetch = fetch;
window.fetch = (...args) => Promise.resolve(_fetch(...args));

@zloirock zloirock closed this as completed Mar 4, 2016
@facundocabrera
Copy link
Author

Looks nice, but not implementable. Thanks all for take time to answer 😄.

@LeoniePhiline
Copy link

What can I say... If native promise implementation is incorrect - it should be replaced. Most alternative Promise polyfills do the same. Someone just replaces global Promise, for example, to Bluebird. So anyway instanceof detection for most cases makes no sense. In your code you can just wrap fetch and have expected instance:

var _fetch = fetch;
window.fetch = (...args) => Promise.resolve(_fetch(...args));

I had to also wrap the Promise-lookalike returned by .json() in MS Edge (15/16/17), in order to be able to use .finally() on its parsed response:

        fetch(url)
            .then(response => {

                let nativeFetchThenable = response.json();

                // In MS Edge, fetch().json() returns a non-Promise thenable, which cannot be polyfilled with .finally().
                // Therefore, we wrap the thenable by resolving a new (polyfilled) Promise with it,
                // so we can call .finally() on the Promise:
                let wrappedFetchPromise = Promise.resolve(nativeFetchThenable);

                wrappedFetchPromise.then(data => myLocalData = data).finally(() => {
                        doSomething(myLocalData);
                    }
                );

            })
            .catch(error => {
                myLocalData = [];
                console && console.error(error);
            });

@Yaffle
Copy link

Yaffle commented Nov 14, 2018

What can I say... If native promise implementation is incorrect - it should be replaced.

It is correct enough for some applications.
This issue is really annoying. Promise#finally polyfills do not work in case core-js is used.
Is there a way to replace fetch conditionally? Seems, no.

@loganfsmyth
Copy link
Contributor

@Yaffle No-one has shown an example of what is actually not working, so if you have one, it would be more helpful to have an example to discuss.

@Yaffle
Copy link

Yaffle commented Nov 14, 2018

@loganfsmyth Yaffle/EventSource#118
in short: I need to have "Promise#finally", I have a polyfill for it. I am using fetch(url).finally(...).

@zloirock
Copy link
Owner

I added a workaround which should fix a big part of cases by patching .than and adding .finally on / to native Promise prototype.

@testacode
Copy link

testacode commented Sep 25, 2020

I'm having this issue also on Microsoft Edge version 44.17763.831.0 and Firefox version 68.12.0esr (32-bit).

I'm using 'defaults' for targeting browserlists and the following:

  • regenerator-runtime 0.13.5
  • @babel/core 7.10.5
  • @babel/preset-env 7.10.4
  • core-js: "^3.6.5"
  • "whatwg-fetch": "^3.2.0"
  • "babel-loader": "^8.1.0",
  • "webpack": "^4.43.0",

my webpack config is the following:

const baseConfig = {
  entry: {
    app: [INDEX_JS_PATH]
  },
  resolve: {
    extensions: ['.js'],
    modules: [NODE_MODULES]
  },
  output: {
    path: DIST_PATH,
    filename: path.join('shell', 'static', '[name]-[hash].js'),
    chunkFilename: path.join('shell', 'static', '[name].[hash].js'),
    publicPath: '/'
  },
  plugins: [
    // Clean up "/dist" between each build
    new CleanWebpackPlugin(),
    // Creates CSS sheets
    new MiniCssExtractPlugin({
      filename: path.join('shell', 'static', 'app.[hash].css'),
      chunkFilename: '[id].css'
    }),
    // Creates "index.html" into "/dist"
    new HtmlWebpackPlugin({
      filename: `./${packageId}/index.html`,
      template: path.resolve(cwd, './src/index.html')
    })
  ],
  module: {
    rules: [
      {
        test: /\.js$/,
        include: [SRC_PATH],
        use: [
          {
            loader: require.resolve('babel-loader'),
            options: {
              babelrc: false,
              cacheDirectory: true,
              // Plugins run before presets from first to last
              plugins: [
                require.resolve('babel-plugin-styled-components'),
                require.resolve('@babel/plugin-proposal-class-properties')
              ],
              // Presets runtime ordering is reversed
              presets: [
                [
                  require.resolve('@babel/preset-env'),
                  {
                    // https://github.com/zloirock/core-js/blob/master/docs/2019-03-19-core-js-3-babel-and-a-look-into-the-future.md
                    useBuiltIns: 'usage',
                    corejs: '3.6',
                    targets: 'defaults'
                  }
                ],
                [
                  require.resolve('@babel/preset-react'),
                  {
                    development: process.env.BABEL_ENV === 'development'
                  }
                ]
              ]
            }
          }
        ]
      },
      {
        test: /\.css$/i,
        use: [
          {
            loader: MiniCssExtractPlugin.loader,
            options: {
              hmr: process.env.NODE_ENV === 'development'
            }
          },
          'css-loader'
        ]
      },
      {
        test: /\.(png|jpg|gif|svg|eot|ttf|woff|woff2)$/i,
        loader: 'file-loader',
        options: {
          outputPath: path.join('shell', 'static'),
          name: '[path][name].[ext]'
        }
      }
    ]
  }
};

I'm going crazy

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

No branches or pull requests

6 participants