Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

fix(graphql-types): graphql-tag binding for default export #80

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

emmenko
Copy link
Contributor

@emmenko emmenko commented May 3, 2018

Labels:

  • bug

Description

Fix bindings for the default export of graphql-tag

screen shot 2018-05-03 at 22 04 11

Instead it should be

var graphqlMutationAST = GraphqlTag.default(Config[/* query */0]);

@@ -2,7 +2,7 @@ open ReasonApolloTypes;

module MutationFactory = (Config:Config) => {
external cast : string => {. "data": Js.Json.t, "loading": bool} = "%identity";
[@bs.module] external gql : ReasonApolloTypes.gql = "graphql-tag";
[@bs.module "graphql-tag"] external gql : ReasonApolloTypes.gql = "default";
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

They seem to have updated the docs showing the difference: https://bucklescript.github.io/docs/en/import-export.html#import-an-es6-default-value

@Gregoirevda
Copy link
Contributor

Gregoirevda commented May 3, 2018

Do you have

{
/* ... */
"package-specs": [
    {
      "module": "commonjs",
    }
  ],
}

in your bsconfig.json?

@emmenko
Copy link
Contributor Author

emmenko commented May 3, 2018

Ah yeah I have "module": "es6".

Hmm, is there anything we can do to support both? 🤔 Actually both are supported (see comment below).

@Gregoirevda
Copy link
Contributor

Does this fixes the problem?
[@bs.module "graphql-tag"] external gql : ReasonApolloTypes.gql = "default";

I'd find it strange cause it should really be exactly the same.

@emmenko
Copy link
Contributor Author

emmenko commented May 3, 2018

Yes this fixes it.

If you look at graphql-tag, it explicitly exports default, so this binding should work in both commonjs and es6.

What do you think?

@Gregoirevda
Copy link
Contributor

The creator of Bucklescript told me ES6 default import support was not merged in master yet. Let's wait a bit more

@emmenko
Copy link
Contributor Author

emmenko commented May 21, 2018

Should we merge the fix first and refactor it once BS has support for that?

Everyone who uses ES6 currently can’t use this library. I understand your point but I find this bug fix important.
Can we maybe publish this under next as well?

@Gregoirevda
Copy link
Contributor

Sure, I'll do that this evening ;)

@emmenko
Copy link
Contributor Author

emmenko commented Jun 18, 2018

@Gregoirevda any update on this one?

@lecler-i
Copy link

Hello, any updates on this one ? The patch works well in my case.. would love to have this released :-)

@Gregoirevda
Copy link
Contributor

This is still an issue for "module": "es6" ?

@baldurh
Copy link

baldurh commented Feb 12, 2019

This is still an issue for "module": "es6" ?

@Gregoirevda yes. I’m battling this currently.

The docs show different examples of importing default and ES6 (babel) default:
https://bucklescript.github.io/docs/en/interop-cheatsheet#import-default

@Gregoirevda
Copy link
Contributor

Is this still an issue? I believe Buckescript now has support for default export...?
Should it be external myFunction : unit -> unit = "my-module" [@@bs.default]

@baransu
Copy link
Contributor

baransu commented Mar 30, 2020

@Gregoirevda Is @bs.default documented anywhere? I cannot find it.

@Gregoirevda
Copy link
Contributor

Can someone confirm this is fixed with es6 modules?
We actually already have this PR merged from another one. What we use is
[@bs.module "graphql-tag"] external gql: gql = "default";
Instead of [@bs.module] external gql: gql = "graphql-tag";

@baransu I was referring to: rescript-lang/rescript-compiler#2677 (comment) which might be outdated

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants