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

ES module import is incompatible with Typings and should not be default #31

Closed
evans opened this issue Jul 18, 2017 · 11 comments
Closed

Comments

@evans
Copy link

evans commented Jul 18, 2017

The current manner of ES imports is incompatible with the Node and Browser imports and @types/zen-observable

This manner of import should be supported and recommended.

import * as Observable from 'zen-observable';
@zenparsing
Copy link
Owner

I think the type declarations in that repository are incorrect. of and from are static (class) methods on the Observable constructor. They are not standalone functions in a module.

See https://github.com/tc39/proposal-observable#observable for more.

@evans
Copy link
Author

evans commented Jul 26, 2017

@zenparsing you are correct about the typings. I opened a PR.

The more important resolution for my project is the lack of consistent exporting between the different ways of constructing javascript modules. Currently the CommonJS export is incompatible with the ES6 export, which causes zen-observable to break depending on how you bundle. CommonJS, used in index.js, exports directly, while the ES6 style in default.js exports as a default. Please let me know if I am misunderstanding something.

Your first iteration of default.js works great!

@zenparsing
Copy link
Owner

Currently the CommonJS export is incompatible with the ES6 export

I don't quite understand. The CJS "version" uses module.exports and the ES "version" uses export default. They are about as equivalent as possible. In fact, the export default thing was primarily added to ES modules in order to emulate the module.exports style.

which causes zen-observable to break depending on how you bundle

What tools are currently breaking?

@evans
Copy link
Author

evans commented Jul 31, 2017

The issues crop up when the code/package is handled by something like webpack, whcih uses the modules field in package.json over main for module resolution. With that type of resolution, the value returned from require contains only a default field. In a node testing environment, the observable is returned directly with the same code, since node uses the file specified by main. Unfortunately, I don't have much insight into how the bundling tools make the decision to use main or module.

Since my code is static on npm and it wasn't feasible to implement two different versions(for main and module), I ended up creating a typescript implementation, so I'm unblocked now and no pressure for a resolution. Thank you for writing a great implementation of observables!

I created an example repo that gives you a typescript example(single test) of how I might have used zen-observable. Hope all this helps!

tsc output from import Observable from 'zen-observable':

Object.defineProperty(exports, "__esModule", { value: true });
var zen_observable_1 = require("zen-observable");
describe('Exports', function () {
    it('constructor', function () {
        chai_1.assert(zen_observable_1.default.of(1));
    });
});

tsc output from import * as Observable from 'zen-observable':

Object.defineProperty(exports, "__esModule", { value: true });
var Observable = require("zen-observable");
describe('Exports', function () {
    it('constructor', function () {
        chai_1.assert(Observable.of(1));
    });
});

@hollowdoor
Copy link

@evans
Copy link
Author

evans commented Sep 19, 2017

@hollowdoor Thank you for the suggestion! I was testing with configurations using browserify as well as webpack and babel, since my library is used in a bunch of different ecosystems. The issue is that I don't know how my library will be included and bundler might pull from either entry point, which leads to different behavior.

@hollowdoor
Copy link

hollowdoor commented Sep 20, 2017

@evans This is one reason why I prefer rollup for the time being. Though it also puts default on a property if there is more than one export. Rollup is still more consistent for pre-transpiling a module.

@evans
Copy link
Author

evans commented Sep 20, 2017

@hollowdoor That's awesome! We happen to be moving towards using rollup in apollo-link #57, which means we might be able to use this zen-observable again.

@hollowdoor
Copy link

@evans Cool. I also recommend buble (in the rollup plugin) as an alternative from babel for transpiling modules as the bundles are much smaller with buble. That's if you aren't using generators, or async await because buble does not handle those. buble also doesn't include polyfills so you'd have to get those yourself. But that's a whole other thing. :)

I would say babel can be better for app level transpilation. Especially if you are using generators, or async await. Unless you just prefer TypeScript. It's up to how you run your projects, and who's all involved.

@evans
Copy link
Author

evans commented Sep 22, 2017

@hollowdoor Thank you for the pointer! We are currently married to Typescript, so I'll have to try buble for a side project.

@justingrant
Copy link

Hi @evans - Were you the author of zen-observable-ts, and if so do you know if there's a successor package? I'm asking because its repo is now archived and there's a bug (aws-amplify/amplify-js#7229) that's affecting a popular AWS client library (amplify-js) that depends on zen-observable-ts. The bug would be trivial to fix but not if the package itself is an orphan! Do you know if this package lives on anywhere in GitHub? If so then I'll file a PR to fix the bug there and to redirect the AWS dependency over to the new package.

If not, then I assume it's a harder lift, e.g. getting this AWS repo working (including react native, angular, react, etc. and multiple bundlers) on top of zen-observable and removing the -ts dependency from Amplify.

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

4 participants