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

Use export default xs to enable autoimport in VSCode #242

Merged
merged 1 commit into from
May 18, 2018

Conversation

Nitive
Copy link
Contributor

@Nitive Nitive commented Apr 8, 2018

vscode-xs-import-demo

@staltz
Copy link
Owner

staltz commented May 18, 2018

Nice! Thanks

@staltz staltz merged commit d3f8df3 into staltz:master May 18, 2018
@Nitive Nitive deleted the autoimport-xs branch May 19, 2018 08:57
@ryota-ka
Copy link
Contributor

This change possibly breaks the existing code.
Before as PR, the default export of the module was Stream as both of its value and its type, but since v1.13.0 only its value is exported.

This is a breaking change for the code which utilises the default-exported stuff as Stream type (not only as a value).

For example, I looked into lib/ directory of staltz/cycle-onionify, to find the following code in pickMerge.d.ts.

import xs from 'xstream';
import { InternalInstances } from './types';
export declare function pickMerge(selector: string): (inst$: xs<InternalInstances<any>>) => xs<any>;

This doesn't compile now as xs cannot use as Stream type.

@staltz
Copy link
Owner

staltz commented May 23, 2018

Very good point @ryota-ka
I hadn't intended xs<T> to be a valid use of xstream, but since enough people used that, it's a breaking change for them.

@staltz
Copy link
Owner

staltz commented May 23, 2018

That said, it's perceivable only for TypeScript users who directly use it. Those who indirectly use it (like in onionify lib/pickMerge.d.ts) can reinstall/recompile and no error would happen.

@ryota-ka
Copy link
Contributor

@staltz Then any plans to release a new version of cycle-onionify with recompiled lib/*.d.ts?
A project relying on it reports errors as below and doesn't compile.

node_modules/cycle-onionify/lib/pickCombine.d.ts(3,64): error TS2304: Cannot find name 'xs'.
node_modules/cycle-onionify/lib/pickCombine.d.ts(3,95): error TS2304: Cannot find name 'xs'.
node_modules/cycle-onionify/lib/pickMerge.d.ts(3,62): error TS2304: Cannot find name 'xs'.
node_modules/cycle-onionify/lib/pickMerge.d.ts(3,93): error TS2304: Cannot find name 'xs'.

@staltz
Copy link
Owner

staltz commented May 24, 2018

Thanks! I released onionify v5.1.0 can you test it?

@ryota-ka
Copy link
Contributor

@staltz Thank you for your quick response and action.
I tested the new version and everything seems perfect now 🎉

@Nitive
Copy link
Contributor Author

Nitive commented May 24, 2018

The breaking change can be fixed by creating a type along with a constant

  const xs = Stream;
+ type xs<T> = Stream<T>;
  export default xs;

@staltz I can create a PR if you like. I think it's better to fix breaking change now and perhaps remove type export in the next major release

@staltz
Copy link
Owner

staltz commented May 26, 2018

@Nitive That's a wise idea. Let's do that, the PR would be welcomed.

Nitive added a commit to Nitive/xstream that referenced this pull request May 28, 2018
Fix breaking change when default-exported stuff is used as as Stream type, not only as a value staltz#242
@Nitive Nitive mentioned this pull request May 28, 2018
staltz pushed a commit that referenced this pull request May 29, 2018
Fix breaking change when default-exported stuff is used as as Stream type, not only as a value #242
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

Successfully merging this pull request may close these issues.

3 participants