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

Issue migrating to TypeScript #114

Closed
groovytron opened this issue Feb 5, 2023 · 10 comments
Closed

Issue migrating to TypeScript #114

groovytron opened this issue Feb 5, 2023 · 10 comments

Comments

@groovytron
Copy link

Hi. I'm trying to migrate this generator to TypeScript. I get the following error when trying to migrate this generator to TypeScript:

npm run test

> [email protected] test
> node --experimental-vm-modules node_modules/jest/bin/jest.js

 FAIL  __tests__/app.spec.ts
  ● Test suite failed to run

    ReferenceError: exports is not defined

      3 | import { createHelpers } from 'yeoman-test';
      4 |
    > 5 | describe('license:app', () => {
        |                       ^
      6 |   let runResult: any;
      7 |   const helpers = createHelpers({});
      8 |

      at __tests__/app.spec.ts:5:23

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        1.232 s
Ran all test suites.
(node:47283) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

Building the generator with npm run build works. And the built package is executed as before. I'm just encountering an error in the tests. There might be an issue in the Jest configuration. I managed to fix it in a project using mocha but I'm not a Jest expert.

The branch to test the problem is there: https://github.com/groovytron/generator-license/tree/migrate-to-typescript

Maybe @mischah could give us a hand on this.

Thanks for your help.

@jozefizso
Copy link
Owner

What issue does TypeScript solve here? The app and code is stable for years and well tested.

@groovytron
Copy link
Author

I'm trying to upgrade yeoman/generator-generator (cf. this issue for more details) to use the latest version of yeoman-generator (which is 5.7.0 at the time of this writing).

As yeoman/generator-generator depends on generator-licenses, I started to try to migrate generator-license to TypeScript first.

The end result should be the same. There is no change on the end user point of view. It's just a dependencies upgrade.

And the tests remains the same which means nothing that was tested in the current version will break in the next version.

I hope it helps to understand the idea of this migration. I don't want to enforce it. I can still migrate my own fork if migrating to TypeScript is a no go.

@jozefizso
Copy link
Owner

I checked the https://github.com/yeoman/generator-generator source code and I don't see any reference to to this package there.

If this package is to be used from TypeScript code a type definition file can be created. I don't see a point in rewriting this app and make it much more complicated with additional tools for compilation when it is working fine.

@mischah
Copy link

mischah commented Feb 6, 2023

I checked the https://github.com/yeoman/generator-generator source code and I don't see any reference to to this package there.

Just to let you know:
It’s a transitive dependency. Buried not that deep in the dependency tree:
generator-generatorgenerator-node → generator-license

Regarding TypeScript in the Yeoman generator eco system.
Using Typescript is not necessary at all. But it’s so damn comfortable to have a much more useful autocompletion in your Editor. So people using your generator when composing their generators would benefit from that. And yes, the additional built step sucks.

Chhers, Michael

@jozefizso
Copy link
Owner

So it's for these three lines https://github.com/yeoman/generator-node/blob/25d1ca4ea2122ce08b88398848e6df41f54cbe1a/generators/app/index.js#L363-L366

That can be done by type definition and not by requiring the whole project to be change to a different toolset.

@jozefizso
Copy link
Owner

And the intellisense is already available:

const lic = require('generator-license');
const x = lic.licenses;

image

@groovytron
Copy link
Author

@jozefizso I completely understand your point and in that case I would suggest I will just maintain a TypeScript fork so that I don't disturb your work. @mischah I will then update the dependencies to point to a package like @groovytron/generator-license in generator-license if that's fine for you. So that we have a solution that satisfies everybody.

And I'll see if I can move this issue to my fork. Otherwise I'll just copy it into mine.

Is that fine for everybody ?

Thanks for your inputs.

@jozefizso jozefizso closed this as not planned Won't fix, can't repro, duplicate, stale Feb 17, 2023
@mischah
Copy link

mischah commented Feb 17, 2023

A fork doesn’t make sense in this case. Since This project is maintained. Why don’t you use this as it is? And create a PR adding type definitions in case @jozefizso will accept to merge this.

What’s your pain with having a JavaScript dependency in a TypeScript Repo?

@jozefizso
Copy link
Owner

I will accept TypeScript type definitions for this repo. The change to TypeScript tooling is our of question.

@groovytron
Copy link
Author

I am really sorry. I didn't understand that having the types definition would actually solve the issue. Thanks for the explaination

I will then add the types definitions and open a PR for this. Thanks for the help and have both a nice weekend.

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

3 participants