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.

Sign up for GitHub

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

Jump to bottom

Addendum to #11... adding TS test #12

Closed
wants to merge 3 commits into from
Closed

Addendum to #11... adding TS test #12

wants to merge 3 commits into from

Conversation

benlesh
Copy link
Owner

@benlesh benlesh commented May 28, 2016

Attn @robwormald... this is an extension on your PR #11, including your commits. Just adding a test to see if everything is kosher.

@benlesh benlesh mentioned this pull request May 28, 2016
Closed
@robwormald
Copy link
Contributor

The problem I'm seeing has to do with how TS vs JS treat the import. I think it's the old .default issue 😐

@benlesh
Copy link
Owner Author

benlesh commented May 28, 2016

yup

robwormald and others added 2 commits May 28, 2016 13:38
@robwormald @benlesh
fix(types): use default syntax for typedef
4c77e54 
this will enable projects consuming symbol-observable from TS (eg, RxJS) to use
```typescript
import $$observable from 'symbol-observable'
```
rather than
```typescript
import * as $$observable from 'symbol-observable'
```

This might? require downstream changes on RxJS but its more correct methinks.
@benlesh
test(typescript): add TypeScript test to ensure it imports and compil…
4046dad 
…es okay
@benlesh
Copy link
Owner Author

benlesh commented May 28, 2016 •
edited
Loading

Okay, so digging into this, it's not possible do support both types of exports. I can't assign a default property on a Symbol. and I can't use Object.defineProperty on a non-object (symbol).

So we're stuck with exporting a named variable, or just exporting default so CJS users get the require('symbol-observable').default thing or they have require('symbol-observable').symbolObservable or something. :\

Yay modules.

@benlesh
Copy link
Owner Author

benlesh commented May 28, 2016

cc/ @gaearon .... what do you think?

@benlesh
Copy link
Owner Author

benlesh commented Jun 1, 2016

So I need to fix this for RxJS and Angular, for sure... @gaearon, you're the only other "big customer", do you have a preference on how to solve this problem? (also would you like commit rights on this repo?)

@benlesh
Copy link
Owner Author

benlesh commented Jun 1, 2016

The proposed solutions are:

  1. CJS uses require('symbol-observable').default
  2. CJS uses require('symbol-observable').symbolObservable (or something like that)

@gaearon
Copy link
Contributor

gaearon commented Jun 12, 2016

I think .default is reasonable.

@robwormald
Copy link
Contributor

Agreed, lets optimize for the future

@benlesh
Copy link
Owner Author

benlesh commented Jun 13, 2016

Closing in favor of #15

@benlesh benlesh closed this Jun 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benlesh @robwormald @gaearon