[plugin-observable] Observables not working as expected

brianmhunt commented 6 years ago

Hi again :)

Observables aren't working as expected. In particular, I've a TC39 implementation of Observables which is not being interpreted correctly.

The problem would seem to be the isObservable function, which adds this to jss:

  var symbolObservable = lib;

  var isObservable = (function (value) {
    return value && value[symbolObservable] && value === value[symbolObservable]();
  });

Here's some command line output that's illuminating:

>>> Symbol.observable === symbolObservable
false
>>> symbolObservable.default === Symbol.observable
true

It's possible there's some weird things going on with the build process on my end (perhaps I need some webpack-commonjs bridge), but I didn't see any tests in jss that demonstrate the use of Symbol.observable proper, so I can't easily eliminate the possibility that it's a jss issue.

kof commented 6 years ago

Can symbol observable package be a problem? So either there was a change and versions are incompatible (I doubt that) or you evtl compiled 2 different versions and they have different refs.

In any case we need to address that, I hope you can come up with a change or a PR for this.

kof commented 6 years ago

symbolObservable.default === Symbol.observable

this sounds like you got a problem with a mix of esm and cjs.

brianmhunt commented 6 years ago

Thanks. The build-process is as vanilla a Webpack 4 config as one could imagine, so it should do CommonJs as expected, but evidently ... something's awry.

Incidentally, it would be quite a bit easier to diagnose if there were a base jsFiddle or CodePen.

brianmhunt commented 6 years ago

FWIW, this looks like a build issue; but a weird one. For interest, the webpack output is

  unwrapExports(lib);

  var symbolObservable = lib;

But it should be:

  var symbolObservable = unwrapExports(lib);

Will report as I work through it.

brianmhunt commented 6 years ago

Relatedly, when my code has this:

>>> import $$obs from 'symbol-observable'
>>> console.log("OBS", $$obs)
OBS Symbol(observable)

i.e. the import is only an issue when it's via the jss library.

brianmhunt commented 6 years ago

The problem seems to exhibit in jss/dist/jss, which indicates a problem with the jss build.

kof commented 6 years ago

so you are using a dist version?

brianmhunt commented 6 years ago

Both are exhibiting the issue.

brianmhunt commented 6 years ago

i.e. both of these fail:

import jss from 'jss'
import jss from 'jss/dist/jss'

brianmhunt commented 6 years ago

... going to check a CDN version now.

kof commented 6 years ago

I guess it will be a problem with dist version if youb have a separate bundle and jss bundle has the symbol built in, so those 2 bundles end up using different symbols.

kof commented 6 years ago

on a lib version, the should be using the same symbol.

brianmhunt commented 6 years ago

The symbol-observable polyfill will always work on a browser where Symbol.observable is defined, since that's what it returns.

Incidentally, unless I'm missing something really obvious, jss is failing from CDN:

<!DOCTYPE html>
<html>
  <head>
    <script src='https://cdnjs.cloudflare.com/ajax/libs/jss/9.8.6/jss.js'></script>
  </head>
  <body>
  </body>
</html>

Console outputs:

Uncaught ReferenceError: global is not defined at escape.js:1 at _typeof (jss.js:4) at jss.js:5 (anonymous) @ escape.js:1 _typeof @ jss.js:4 (anonymous) @ jss.js:5

Which is the following line from escape.css:

const CSS = (global.CSS: any)

So I can't seem to test with the CDN either. Maybe a caching issue or something since I've been fiddling locally.

kof commented 6 years ago

oh, migration to rollup broke that, webpack has a built-in plugin for "global" replacement

brianmhunt commented 6 years ago

Yes, these seem like the types of issues one gets with Rollup. :) We had similar problems with tko/knockout. For interest, our rollup config

kof commented 6 years ago

looks like we need to add https://www.npmjs.com/package/rollup-plugin-node-globals

kof commented 6 years ago

This won't fix the prob with symbols though

brianmhunt commented 6 years ago

Unfortunately without a control (jsFiddle/CodePen, jss via CDN), we can't eliminate the jss build as the problem. Having stared at it for a couple hours, I do suspect it's the jss build process — but before encouraging a revision of that process I'd like to eliminate the possibility that it's a problem on my end by checking what a CDN version uses for its symbolObservable.

So step one would seem to be fixing the global symbols.

kof commented 6 years ago

You can try in the meantime an older version, which used webpack

brianmhunt commented 6 years ago

Thanks @kof; what's a recent version compiled with webpack?

kof commented 6 years ago

https://github.com/cssinjs/jss/blob/master/changelog.md#981--2018-03-19

brianmhunt commented 6 years ago

TC39 observables appear to work as expected with 9.8.1, indicating that the Rollup config is the culprit.

Probably not relevant, but here's the Webpack version of isObservable.js:

Object.defineProperty(exports, "__esModule", {
  value: true
});

var _symbolObservable = require('symbol-observable');

var _symbolObservable2 = _interopRequireDefault(_symbolObservable);

function _interopRequireDefault(obj) {
  return obj && obj.__esModule ? obj : { 'default': obj }; }

exports['default'] = function (value) {
  return value && value[_symbolObservable2['default']] &&
     value === value[_symbolObservable2['default']]();
};

In contrast to the Rollup version set out in this issue's description, in this Webpacked version we get what is the expected result from dev tools inspection:

>>> _symbolObservable2['default'] === Symbol.observable
true

kmturley commented 5 years ago

If you get the error global is not defined you can add this code to your src/polyfills.ts

// Add global to window, assigning the value of window itself.
(window as any).global = window;

keeth commented 3 years ago

Has anyone had any luck with this? Just ran into it 3 years later.

cssinjs / jss

[plugin-observable] Observables not working as expected #735