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

Invalid es6 code generated when binding to module typed as function #2456

Closed
glennsl opened this issue Jan 21, 2018 · 15 comments
Closed

Invalid es6 code generated when binding to module typed as function #2456

glennsl opened this issue Jan 21, 2018 · 15 comments

Comments

@glennsl
Copy link
Contributor

glennsl commented Jan 21, 2018

tl;dr: import * as Foo from 'foo'; is not equivalent to import Foo from 'foo';

When compiling to es6-global a module external typed as a function, like

external myFunction : unit -> unit = "my-module" [@@bs.module]

will generate the import

import * as MyFunction from 'my-module';

which is incorrect since MyFunction imported in this manner is defined as a namespace object. See https://www.ecma-international.org/ecma-262/6.0/#sec-module-namespace-objects

It should generate

import MyFunction from 'my-module';

This causes standards-conforming tools like rollup to throw a fit, which is a pretty serious problem.

My workaround for now is:

[%%raw "import MyFunction from 'my-module'"];
external myFunction : unit -> unit = "MyFunction" [@bs.val] ;
let myFunction = myFunction;
@bobzhang bobzhang added the bug label Jan 22, 2018
@bobzhang
Copy link
Member

microsoft/TypeScript#2719
Is this the same issue: default import/export?
microsoft/TypeScript#2719 (comment)

@glennsl
Copy link
Contributor Author

glennsl commented Jan 22, 2018

It might be related, but this issue is about the import semantics, not the export semantics.

It could be interpreted as import * as foo from 'bar' being (roughly) equivalent to var foo = require('bar'), and import foo from 'bar' being equivalent to var foo = require('bar').default, but it's not completely clear to me.

Here's one of many issues about it in the rollup repo: rollup/rollup#670

@thangngoc89
Copy link
Contributor

Webpack relies on this behavior for tree shaking too.
Writing import * as foo from 'bar' means webpack will put everything inside bar module into the bundle.

@bobzhang
Copy link
Member

bobzhang commented Mar 1, 2018

This is currently what we have:

import * as MyModule from "my-module";
MyModule();

I am a bit confused, is not import MyModule from "my-module" default import? What's the correct way to grab the whole es6 module?

Edit: sorry, I am a bit dumb here, is not what we generated correct here? [@@bs.module] is supposed to be binding to �a Namespace object?

@bobzhang bobzhang added priority:high and removed bug labels Mar 1, 2018
@glennsl
Copy link
Contributor Author

glennsl commented Mar 1, 2018

Default import:

import MyModule from 'my-module';

Namespace object (or "whole module", if you will)

import * as MyModule from 'my-module';

@mrmurphy
Copy link

mrmurphy commented Mar 14, 2018

This is causing me troubles right now, as I'm trying to use Rollup to bundle my code before deploying with firebase functions (related to #2127).

But Rollup is throwing a namespace error when using the library bs-express:

index.js → bundle.js...
[!] Error: Cannot call a namespace ('Express')
node_modules/bs-express/src/Express.js (633:9)
631:
632: function express() {
633:   return Express();
              ^
634: }

Is my only option right now to fork bs-express and use @glennsl's workaround?
Thanks!

@glennsl
Copy link
Contributor Author

glennsl commented Mar 14, 2018

@splodingsocks are you sure you need to use rollup? webpack should work, unless you have very specific needs that only rollup can solve.

@mrmurphy
Copy link

Oh, I've never used Webpack for bundling libraries that expose functions before @glennsl. Firebase functions work by looking for the exports from the index.js file and hosting those as publicly accessible functions. I can certainly look into doing that with Webpack, though. I imagine it's possible?

@glennsl
Copy link
Contributor Author

glennsl commented Mar 15, 2018

I have no reason to believe it behaves any differently than rollup in this regard, but I also haven't tried it so I don't know.

You might also be able to use the commonjs rollup plugin to work around this.

cyberhuman added a commit to cyberhuman/bs-express that referenced this issue Mar 15, 2018
@glennsl glennsl closed this as completed Mar 24, 2018
@thangngoc89
Copy link
Contributor

@glennsl why did you close this?

@glennsl
Copy link
Contributor Author

glennsl commented Mar 24, 2018

Bob doesn't consider it a bug, and I no longer care. I think he was going to create a new issue for default import support.

@ozanmakes
Copy link

ozanmakes commented Mar 26, 2018

I've just hit this one as well. I think one of the entries in the interop cheatsheet is relevant:

Import ES6 default compiled from Babel:
external studentName: string = "default" [@@bs.module "./student"]
Reason syntax:
[@bs.module "./student"] external studentName : string = "default";

With this one I'm getting the following output, which works with Babel:

import * as myModule from 'my-module';
myModule.default();

But the problem with both this method and the one described in the issue description is, users of these bindings won't be able to use commonjs output anymore.

How about something like this for ES6 modules:

import myDefault, * as myModule from 'my-module';

cyberhuman added a commit to cyberhuman/bs-express that referenced this issue Apr 6, 2018
@guilhermedecampo
Copy link

Hey guys don't think this should be closed.

I'm also having issues.

Trying some bindings for aws-amplify. They have a configure method in the root as default and Auth and other objects being exported.

So for now I have for example:

type auth;

[@bs.module "aws-amplify"]
external config :
  {
    .
    "Auth": {
      .
      "identityPoolId": string,
      "region": string,
    },
  } =>
  Js.Promise.t(Js.t({..})) =
  "configure";

let configure = (~identityPoolId: string, ~region: string) =>
  config({
    "Auth": {
      "identityPoolId": identityPoolId,
      "region": region,
    },
  });

[@bs.module "aws-amplify"] external auth : auth = "Auth";

[@bs.send] external signOut' : auth => Js.Promise.t(unit) = "signOut";

let signOut = () => signOut'(auth);

[@bs.send]
external signIn' :
  (auth, ~username: string, ~password: string) => Js.Promise.t(Js.t({..})) =
  "signIn";

let signIn = (~username, ~password) => signIn'(~username, ~password, auth);

That generates

// Generated by BUCKLESCRIPT VERSION 2.2.3, PLEASE EDIT WITH CARE

import * as AwsAmplify from "aws-amplify";

function configure(identityPoolId, region) {
  return AwsAmplify.configure({
              Auth: {
                identityPoolId: identityPoolId,
                region: region
              }
            });
}

function signOut() {
  return AwsAmplify.Auth.signOut();
}

export {
  configure ,
  signOut ,
  
}
/* aws-amplify Not a pure module */

Spent some good time to get into this issue and find the @glennsl 's workaround.

cyberhuman added a commit to cyberhuman/bs-node-fetch that referenced this issue Apr 12, 2018
cyberhuman added a commit to cyberhuman/bs-node-fetch that referenced this issue Apr 12, 2018
@bobzhang
Copy link
Member

@guilhermedecampo moved to #2677

For people who are watching this thread, this is not a bug, default ES6 import is not supported yet, bs.module is not for default import, we are working on it in #2677

@guilhermedecampo
Copy link

Ok thanks!

ctbucha added a commit to ctbucha/bs-react-admin that referenced this issue Aug 29, 2018
* added initial package.json

* installed dependencies, added npm scripts, and added gitignore file.

* added ra-data-json-server and react-admin.Admin

* added react-admin.Resource and bsconfig.json

* fixed typo

* trying to get default export to work

* used bucklescript workaround to get the default field to work (rescript-lang/rescript#2456)

* fixed errors in react-admin.Resource
cyberhuman added a commit to cyberhuman/bs-node-fetch that referenced this issue Sep 11, 2018
cyberhuman added a commit to cyberhuman/bs-express that referenced this issue Sep 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants