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

Different behaviour than tsc+node when using class-transformer #257

Closed
thevtm opened this issue Jun 30, 2017 · 17 comments
Closed

Different behaviour than tsc+node when using class-transformer #257

thevtm opened this issue Jun 30, 2017 · 17 comments

Comments

@thevtm
Copy link

thevtm commented Jun 30, 2017

First of all great project, I have really been enjoying using it. 😄
I found a weird issue when doing some exploration testing on class-transformer.

I'm using class-transformer to convert a plain object to a classed instance. Like this:

test("This should work", () => {
  class Weapon {
    constructor(public model: string,
                public range: number) { }
  }

  class User {
    public id: number;
    public name: string;

    @Type(() => Weapon)
    public weapons: Map<string, Weapon>;
  }

  const plainUser = {
    id: 1,
    name: "Max Pain",
    weapons: { firstWeapon: { model: "knife",  range: 1 } },
  };

  const classedUser = plainToClass(User, plainUser);
  expect(classedUser).toBeInstanceOf(User);
  expect(classedUser.weapons).toBeInstanceOf(Map);
});

Issue

class-transformer should transform the nested object plainUser.weapons from object to Map<string, Weapon>.
The issue is that when I execute that test it fails, instead of transforming to Map<string, Weapon I get Weapon.
But when I run it regularly (tsc then node) it works.

In test:

classedUser is instance of User? true
classedUser.weapons is instance of Map? false
classedUser.weapons is instance of Weapon? true
 FAIL  ./index.test.ts
  ● This should work

    expect(value).toBeInstanceOf(constructor)
    
    Expected value to be an instance of:
      "Map"
    Received:
      {"firstWeapon": {"model": "knife", "range": 1}, "model": undefined, "range": undefined, "secondWeapon": {"model": "eagle", "range": 200}, "thirdWeapon": {"model": "ak-47", "range": 800}}
    Constructor:
      "Weapon"
      
      at Object.<anonymous> (index.test.ts:50:31)
          at Promise (<anonymous>)
          at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:169:7)

  ✕ This should work (263ms)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        1.915s
Ran all test suites.

Expected behavior

expect(classedUser.weapons).toBeInstanceOf(Map); // Should pass

In node:

classedUser is instance of User? true
classedUser.weapons is instance of Map? true
classedUser.weapons is instance of Weapon? false

Repo

  • npm start runs tsc + node (expected behaviour)
  • npm test runs jest (issue)
  • I included the compiled js
node v8.1.3
jest v20.0.4
ts-jest v20.0.6
typescript v2.4.1
@kulshekhar
Copy link
Owner

I'm not familiar with class-transformer but the following looks odd somehow:

    @Type(() => Weapon)
    public weapons: Map<string, Weapon>;

Assuming @Type is used to store info about the type of the variable, shouldn't it be

    @Type(() => Map)
    public weapons: Map<string, Weapon>;

?

That said, I'm actually a bit baffled because the start script seems to identify weapons as a map while that's not the case during testing, even though both setups use the same tsconfig

@kulshekhar
Copy link
Owner

I looked into this a bit and I don't think this is a ts-jest issue. I've created a repository that illustrates this issue without using ts-jest.

Here's the output of running yarn/npm test on this repo:

Running code transpiled from CLI:
classedUser is instance of User? true
classedUser.weapons is instance of Map? true
classedUser.weapons is instance of Weapon? false

Running code transpiled using the typescript package:
classedUser is instance of User? true
classedUser.weapons is instance of Map? false
classedUser.weapons is instance of Weapon? true

In the first case, the code is transpiled using the command line (and it works as you expect it to)

In the second case, the code is transpiled using the transpile function in the typescript package (source), using the same configuration file. The transpiled output is then executed using node and shows a different output.

Comparing line 30 in the two transpiled files (using command line and using typescript package) might give a hint to what's going wrong.

The version of typescript in both cases was the same - 2.4.1

Unless there's something I'm doing incorrectly when transpiling the code, this seems like an issue with Typescript.

@thevtm
Copy link
Author

thevtm commented Jun 30, 2017

Thank you for the quick response!!
I created an issue for this in the TS repository.

@thevtm
Copy link
Author

thevtm commented Jul 5, 2017

For the record the issue was that ts-jest uses the function transpileModule to transpile.

That is an issue because:

The transpile (and transpileModule) functions unconditionally set isolatedModules, noLib, and noResolve to true. As a result, we cannot resolve Map to its constructor and instead return Object.

@bcruddy
Copy link
Contributor

bcruddy commented Jul 5, 2017

@thevtm interesting - can you suggest an alternate approach? I haven't considering using anything else and going through the typescript source I'm not sure what the options would even be. I left a similar comment in the TypeScript repo on your issue.

It looks like we may need to reopen this @kulshekhar

@kulshekhar
Copy link
Owner

@bcruddy I'm not sure about that.

Unless there's a way to to replicate the behavior of the command line tsc, there's not much we can do here. I've gone through the docs again but couldn't see any other way to transpile TypeScript code that would work in the manner we need.

I don't know the reasons behind the behavioral difference but there shouldn't be any.

@thevtm
Copy link
Author

thevtm commented Jul 5, 2017

I've managed to make it work using ts.createProgram() based on this example from the wiki.

I don't know if it would satisfy the requirements.

Repo

@kulshekhar
Copy link
Owner

@thevtm this option would create the transpiled files on disk, right?

@thevtm
Copy link
Author

thevtm commented Jul 5, 2017

Yes, but I think its possible to change that through program.emit().

/**
 * Emits the JavaScript and declaration files.  If targetSourceFile is not specified, then
 * the JavaScript and declaration files will be produced for all the files in this program.
 * If targetSourceFile is specified, then only the JavaScript and declaration for that
 * specific file will be generated.
 *
 * If writeFile is not specified then the writeFile callback from the compiler host will be
 * used for writing the JavaScript and declaration files.  Otherwise, the writeFile parameter
 * will be invoked when writing the JavaScript and declaration files.
 */
emit(targetSourceFile?: SourceFile, writeFile?: WriteFileCallback, cancellationToken?: CancellationToken, emitOnlyDtsFiles?: boolean, customTransformers?: CustomTransformers): EmitResult;

@kulshekhar
Copy link
Owner

I gave it a quick look but couldn't figure out how to extract the transpiled code from this. Any idea?

@thevtm
Copy link
Author

thevtm commented Jul 5, 2017

Using the writeFile?: WriteFileCallback parameter. 😄

// Compile
const program = ts.createProgram(compilerConfig.fileNames, compilerConfig.options)
let emitResult = program.emit(program.getSourceFile('sample.ts'),
//(fileName: string, data: string, writeByteOrderMark: boolean, onError?: (message: string) => void, sourceFiles?: ts.SourceFile[]): void
  (fileName: string, data: string, writeByteOrderMark: boolean) => {
    console.log(fileName, data)
  })

Repo (branch emit)

@kulshekhar kulshekhar reopened this Jul 5, 2017
@kulshekhar
Copy link
Owner

This might still be a problem. I think jest expects a transformer to return code synchronously

@GeeWee
Copy link
Collaborator

GeeWee commented Jul 7, 2017

It does. Transformers must be synchronous.

@DorianGrey
Copy link
Contributor

Note that although it seems different, program.emit is synchronous - the callback is just called for every file to write. In fact, transpileModule is using it internally:
https://github.com/Microsoft/TypeScript/blob/4b3e661aaa69e53b6a5992c76f2232c9f5dace10/src/services/transpile.ts#L100
Generated code is extracted with the writeFile callback of the compilerHost.
For the purpose mentioned here, it would be required to copy most of the transpileModule implementation, excluding some of the option reset steps (I'm not sure which ones - should be tested).
It is intended that some of these options are removed, to avoid problems with the single file perspective of transpileModule. I'm not sure if this may work in a more complex project.

@floriansimon1
Copy link

floriansimon1 commented Feb 6, 2018

The way files get transpiled makes all libraries that use design:type untestable with ts-jest, as the wrong info gets returned when querying the design:type metadata. There is no possible workaround.

I'd like to state my disagreement with the classification of this problem as a TS bug. TS puts the tools at your disposal (see @thevtm's work, or the language services API), but you're not using them. There is a possibility to make this work, but it's not yet done, for reasons I can understand. I also understand that you don't owe the community anything, but I'd like to make a suggestion.

Maybe it'd be interesting to put a note in the README to let your users know that libraries like typegoose or class-transformer that rely on proper reflection won't work with ts-jest? I can do it if it's fine for you. At least, this would allow your users to not lose time trying to make ts-jest work when it clearly cannot, whatever the reason may be.

@GeeWee
Copy link
Collaborator

GeeWee commented Feb 8, 2018

Would definitely like a PR with this.

I think this has most to do with the fact that jest allows you to preprocess one file at a time, and my impression is that stuff like this needs to have access to the complete type information.

@GeeWee
Copy link
Collaborator

GeeWee commented Mar 4, 2018

Closing this in favour of #439 - reasonably sure the solution will be the same.

@GeeWee GeeWee closed this as completed Mar 4, 2018
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

No branches or pull requests

6 participants