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

Fix Static Land compliance of the TypeRep exposed by Flutures module version #109

Merged
merged 2 commits into from
May 30, 2017

Conversation

Avaq
Copy link
Member

@Avaq Avaq commented May 30, 2017

Other than improving the code organization, this ensures that Future.of and Future.reject are present in module builds as well as UMD builds.

As a result, the following will become possible:

- import {of} from 'fluture'
- of(1)
+ import Future from 'fluture'
+ Future.of(1)

I find that many users expect this to work.

This ensures that Future.of and Future.reject are
present in module builds as well as UMD builds.
@codecov-io
Copy link

codecov-io commented May 30, 2017

Codecov Report

Merging #109 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #109   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          44     45    +1     
  Lines        1069   1074    +5     
=====================================
+ Hits         1069   1074    +5
Impacted Files Coverage Δ
src/chain-rec.js 100% <ø> (ø) ⬆️
src/core.js 100% <ø> (ø) ⬆️
src/future.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c764ae...ae125ed. Read the comment docs.

@Avaq
Copy link
Member Author

Avaq commented May 30, 2017

While reviewing my own code, I realized that for the same reason that of was missing from the TypeRep in module builds, so were all the other Static Land properties. In order to have the Future module be compliant to Static Land, I've added the appropriate dispatchers to the TypeRep.

@Avaq
Copy link
Member Author

Avaq commented May 30, 2017

Would you mind having a peek @rpominov?

@Avaq Avaq changed the title Move TypeRep related stuff to separate module Fix Static Land compliance of the TypeRep exposed by Flutures module version May 30, 2017
Copy link

@rpominov rpominov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

never,
isNever
} from './src/core';
export {default, default as Future} from './src/future';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure that ES specification allows to do export {default} from '...'. It maybe the case that it works with current tools by incident, and may not work in the future.

Copy link
Member Author

@Avaq Avaq May 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading through the spec, one rule is concerning:

For each IdentifierName n in ReferencedBindings of ExportClause: It is a Syntax Error if StringValue of n is a ReservedWord

In the spec for ReservedWord we see that default is actually a reserved word. This would mean that export {Future as default} from '...' also shouldn't work.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Maybe you should do import Future from './src/future'; export default Future; to be safe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means I have to do it like this:

import _Future from './src/future';

export default _Future;
export const Future = _Future;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, ugly. But I would probably do it like this though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the same mistake in other places of the code. I'll merge this PR as is, and create a separate issue to discuss the matter.

@Avaq Avaq merged commit 136d1bc into master May 30, 2017
@Avaq Avaq deleted the avaq/type-rep branch May 30, 2017 11:38
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.

None yet

3 participants