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

Mixing conversions and submodule's isn't using the conversion #414

Closed
tomgasson opened this issue Apr 29, 2020 · 5 comments · Fixed by #416
Closed

Mixing conversions and submodule's isn't using the conversion #414

tomgasson opened this issue Apr 29, 2020 · 5 comments · Fixed by #416

Comments

@tomgasson
Copy link

tomgasson commented Apr 29, 2020

[@genType]
type t =
  | A
  | B({name: string});

module X = {
  [@genType]
  let foo = (t: t) => {
    switch (t) {
    | A => Js.log("A")
    | B({name}) => Js.log("B" ++ name)
    };
  };
};

Gives

type $any = any;

const $$toRE552311971 = {"A": 0};

// $FlowExpectedError: Reason checked type sufficiently
const CreateBucklescriptBlock = require('bs_block');

// $FlowExpectedError: Reason checked type sufficiently
const GenTypeBugBS = require('GenTypeBug.bs');

export type t = "A" | {| +name: string |};

const X_foo: (t) => void = function (Arg1: $any) {
  const result = GenTypeBugBS.X.foo(typeof(Arg1) === 'object'
    ? CreateBucklescriptBlock.__(0, Arg1)
    : $$toRE552311971[Arg1]);
  return result
};;
exports.X_foo = X_foo

const X: { foo: (t) => void, ... } = GenTypeBugBS.X;
exports.X = X

A call to Module.X.foo("A") won't use X_foo, it will use the BS.X.foo method directly

@cristianoc
Copy link
Collaborator

@tomgasson thanks for the report. Spot on.

@cristianoc
Copy link
Collaborator

@tomgasson do you actually need it, or is it just an issue you noticed?

We're moving towards zero conversion once the runtime representation of variants changes.
So here the fix would be to make a copy of the .bs.js module, which goes exactly in the opposite direction of the desired "zero runtime".

I'm tempted to just not generate the internal module when conversion is required.
So, these lines would just not be generated when parts of the module require conversion:

const X: { foo: (t) => void, ... } = GenTypeBugBS.X;
exports.X = X

@tomgasson
Copy link
Author

tomgasson commented Apr 29, 2020

Yeah, this is a real bug we hit. If we're already on a path to not need this that's great. If it's easy, not generating the X.foo type will make it hard to fall for this trap

@cristianoc
Copy link
Collaborator

@tomgasson see fix in #416 where inner modules are not emitted when they require conversion.
If it looks good, I'll go ahead and merge.

@tomgasson
Copy link
Author

Looks good. Thanks for the fix

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 a pull request may close this issue.

2 participants