rescript-lang / rescript-compiler

The compiler for ReScript.
https://rescript-lang.org
Other
6.64k stars 441 forks source link

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

Closed glennsl closed 6 years ago

glennsl commented 6 years ago

tl;dr: `import as Foo from 'foo';is not equivalent toimport 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 commented 6 years ago

https://github.com/Microsoft/TypeScript/issues/2719 Is this the same issue: default import/export? https://github.com/Microsoft/TypeScript/issues/2719#issuecomment-92189669

glennsl commented 6 years ago

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: https://github.com/rollup/rollup/issues/670

thangngoc89 commented 6 years ago

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 commented 6 years ago

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?

glennsl commented 6 years ago

Default import:

import MyModule from 'my-module';

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

import * as MyModule from 'my-module';
mrmurphy commented 6 years ago

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 https://github.com/BuckleScript/bucklescript/issues/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 commented 6 years ago

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

mrmurphy commented 6 years ago

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 commented 6 years ago

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.

thangngoc89 commented 6 years ago

@glennsl why did you close this?

glennsl commented 6 years ago

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 commented 6 years ago

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';
guilhermedecampo commented 6 years ago

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.

bobzhang commented 6 years ago

@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 commented 6 years ago

Ok thanks!