Set up TypeScript #19559

emilpaw · 2022-08-28T15:46:26Z

Initial set up of TypeScript.

Please make sure the below checklist is followed for Pull Requests.

All continuous integration tests are green
Tests are added where necessary
The JDL part is updated if necessary
jhipster-online is updated if necessary
Documentation is added/updated where necessary
Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

When you are still working on the PR, consider converting it to Draft (below reviewers) and adding skip-ci label, you can still see CI build result at your branch.

.eslintrc.json

Tcharl · 2022-09-05T20:55:11Z

Not an expert, but I would love to start contributing to jhipster in typescript!

.gitignore

emilpaw · 2022-09-11T16:36:04Z

At this point running npm run build compiles all the JavaScript code files and moves the needed assets to the build directory dist.

Running it does not work yet though. First linking with npm link and then running jhipster info throws AssertionError [ERR_ASSERTION]: Error on the registered namespace dist:app, make sure your folder is called generator-jhipster.

@mshima Could you help me understand what the error exactly means and what the issue is?

mshima · 2022-09-11T17:51:21Z

At this point running npm run build compiles all the JavaScript code files and moves the needed assets to the build directory dist.

Why copy every js file to dist?
We can keep old structure with additional dist and src folders for now.

Running it does not work yet though. First linking with npm link and then running jhipster info throws AssertionError [ERR_ASSERTION]: Error on the registered namespace dist:app, make sure your folder is called generator-jhipster.

@mshima Could you help me understand what the error exactly means and what the issue is?

This is related to the generators lookup process.
We need to customize the lookup process when moving generators to dist.
IMO we should do this when they are migrated to typescript.

emilpaw · 2022-09-11T21:01:38Z

Why copy every js file to dist?

TypeScript compiles all source files, JavaScript and TypeScript ones. It's not just copying the files. Not sure what the migration to TS could look like if we only compile a selected part of the program and leave the rest uncompiled.

I was thinking about letting TypeScript compile the whole program and then piece after piece migrate the JavaScript files to TypeScript.

What exactly did you have in mind?

This is related to the generators lookup process. We need to customize the lookup process when moving generators to dist.

I am still not sure what the code does but I just naively replace .. with ../.. in

generator-jhipster/cli/environment-builder.js

Line 119 in b741f65

    
           this.env.lookup({ packagePaths: [path.join(__dirname, '..')], lookups: ['generators'] }).forEach(generator => {

and so far it seems to work. At least generating a project works fine.

IMO we should do this when they are migrated to typescript.

If there is a way to only compile parts of the programm and we decide we prefer that over letting TypeScript compile all source files we could do this. Otherwise this is needed to be done within the scope of this PR.

mraible · 2022-09-11T21:25:02Z

Might be relevant to this conversation:

https://blog.jim-nielsen.com/2018/installing-and-building-an-npm-package-from-github/

mshima · 2022-09-11T22:08:58Z

Why copy every js file to dist?

TypeScript compiles all source files, JavaScript and TypeScript ones. It's not just copying the files. Not sure what the migration to TS could look like if we only compile a selected part of the program and leave the rest uncompiled.

I was thinking about letting TypeScript compile the whole program and then piece after piece migrate the JavaScript files to TypeScript.

What exactly did you have in mind?

I was thinking to migrate jdl first to reduce the complexity of the migration.

This is related to the generators lookup process. We need to customize the lookup process when moving generators to dist.

I am still not sure what the code does but I just naively replace .. with ../.. in

generator-jhipster/cli/environment-builder.js

Line 119 in b741f65

this.env.lookup({ packagePaths: [path.join(__dirname, '..')], lookups: ['generators'] }).forEach(generator => {

The lookups should be changed to dist/generators too.

and so far it seems to work. At least generating a project works fine.

IMO we should do this when they are migrated to typescript.

If there is a way to only compile parts of the programm and we decide we prefer that over letting TypeScript compile all source files we could do this. Otherwise this is needed to be done within the scope of this PR.

IFAIK jdl doesn't uses other folder, so compiling the jdl folder and moving it to src/jdl should be enough.
And change jdl imports to use dist/jdl.

generator-jhipster/cli/import-jdl.js

Line 28 in b741f65

    
           const { createImporterFromContent, createImporterFromFiles } = require('../jdl/jdl-importer');

Anyway, feel free to compile everything if it works.

mshima

We will support node >=16, target and module can be bumped.

package.json

tsconfig.json

package.json

mshima · 2022-09-13T22:44:02Z

What do you want to do? Do you want the tests to stay written in JavaScript and not migrated to TypeScript?

If we copy tests to dist, how do we update snapshots?
We should move every directory to src before building with typescript. This will make import package.json work in both dist and src directories.
We could use mocha + ts-node to test ts files or other alternative for in place ts tests.

emilpaw · 2022-09-14T20:11:20Z

If we copy tests to dist, how do we update snapshots?

I haven't yet looked into how snapshots work, so I don't know.

We should move every directory to src before building with typescript. This will make import package.json work in both dist and src directories.

You mean to move cli, generators, jdl and lib into a newly created directory src and then use the path ../../package.json instead of ../package.json? If you think that's better than copying the package.json we can do that.

We could use mocha + ts-node to test ts files or other alternative for in place ts tests.

Yes, I also looked into that but wasn't sure if that's the right approach.

The generator lookup seems to not work correctly yet. At least that's my guess based on the error message in the failed tests. So just changing the paths did not work. I am still looking into that.

mshima · 2022-09-14T22:07:21Z

If we copy tests to dist, how do we update snapshots?

I haven't yet looked into how snapshots work, so I don't know.

npm run update-snapshots runs tests updating snapshots located near tests.

We should move every directory to src before building with typescript. This will make import package.json work in both dist and src directories.

You mean to move cli, generators, jdl and lib into a newly created directory src and then use the path ../../package.json instead of ../package.json? If you think that's better than copying the package.json we can do that.

That's not the only reason, having more than one root source folder can be confusing.
Improving the source tree is planned for v8.
My idea was to have cli, lib and generators folder for v8. Moving utils and jdl inside lib.
Migrating everything to typescript, we can have something like:

src
- cli
- jdl
- generators
- lib
  - utils

We could use mocha + ts-node to test ts files or other alternative for in place ts tests.

Yes, I also looked into that but wasn't sure if that's the right approach.

The generator lookup seems to not work correctly yet. At least that's my guess based on the error message in the failed tests. So just changing the paths did not work. I am still looking into that.

The lookup is working fine, all integration tests are working.
But tests are been executed in place, so the lookup is looking in the wrong place when executing tests due to the change.

Moving sources to src should fix the lookup too.

cli/environment-builder.js

Don't copy files ending with .snap and folders starting with __ (currently __snapshots__ & __workflow) to build output. These are files needed for tests or CI which have no use in the build output.

emilpaw · 2022-09-15T19:06:21Z

All the tests were passing. While rebasing I made a mistake but now I restored it to the pre-rebase mistake state. @mshima You could force push your local branch if commits are missing but I think it's good now.

I also started to work on moving the source files to an src folder. See: #19750

In both PRs the pipelines don't seem to start anymore though. Any idea why?

mshima · 2022-09-15T19:42:25Z

In both PRs the pipelines don't seem to start anymore though. Any idea why?

Due to conflicts, since this tests of this PR is succeeding, we can finish it and merge.
The directory rearrangement can be done after.

test-integration/scripts/10-install-jhipster.sh

emilpaw · 2022-09-15T21:12:30Z

Due to conflicts

I thought that also the pipeline in this PR was not starting but it just took a very long time.

Adapting the package.json exports to the new build output was not yet done. I changed the paths and then some tests broke. Currently, I am working on that. Other than that I think this PR is done.

mshima · 2022-09-16T11:19:51Z

package.json

    "./esm/cli": "./cli/index.mjs",
    "./esm/generators/*": "./generators/*/esm.mjs",
    "./esm/generators": "./lib/constants/generators.mjs",
    "./esm/priorities": "./lib/constants/priorities.mjs",
    "./esm/support": "./lib/support/index.cjs"


Is there a reason for not update?

Suggested change

"./esm/cli": "./cli/index.mjs",

"./esm/generators/*": "./generators/*/esm.mjs",

"./esm/generators": "./lib/constants/generators.mjs",

"./esm/priorities": "./lib/constants/priorities.mjs",

"./esm/support": "./lib/support/index.cjs"

"./esm/cli": "./dist/cli/index.mjs",

"./esm/generators/*": "./dist/generators/*/esm.mjs",

"./esm/generators": "./dist/lib/constants/generators.mjs",

"./esm/priorities": "./dist/lib/constants/priorities.mjs",

"./esm/support": "./dist/lib/support/index.cjs"

Yes, I explained it in the commit: 5263183

package.json

mshima

I took a look at prepare script and it seems to work for this case:
Adding "prepare": "npm run build", script to package.json and build should be executed every time npm install is executed.
Can you check please?

mshima · 2022-09-16T11:22:11Z

test-integration/scripts/10-install-jhipster.sh

    npm ci --prod
+    npm run build


Suggested change

npm ci --prod

npm run build

npm install

mshima · 2022-09-16T11:23:01Z

test-integration/scripts/10-install-jhipster.sh

    npm ci --prod
+    npm run build


Suggested change

npm ci --prod

npm run build

npm install

mshima · 2022-09-16T11:26:32Z

tsconfig.json

+  "include": ["cli/**/*", "generators/**/*", "jdl/**/*", "lib/**/*", "test/**/*"],
+  "exclude": ["**/*.spec.*"],
+  "compilerOptions": {
+    /* Visit https://aka.ms/tsconfig to read more about this file */


I think we can remove commented configs.

When using the build output for these exports the tests which import these modules as part of the test case fail. This has two reasons: 1. In CI/CD npm run build is not run before the tests so the modules fail to be imported. This could be solved by first building the package. 2. In the tests the modules are imported via ES Modules syntax and the import function and the result is compared for referential equality. When the built file is imported through the import function and it's compared to the source file the reference is always not the same. When this part of the code is converted to TypeScript these issues must be addressed.

This will build the project when it's installed.

This should be done by the prepare npm script now.

github-actions bot added the theme: dependencies Pull requests that update a dependency file label Aug 28, 2022

emilpaw mentioned this pull request Aug 28, 2022

Migrate JDL to typescript #19182

Closed

1 task

mshima reviewed Aug 28, 2022

View reviewed changes

.eslintrc.json Outdated Show resolved Hide resolved

mshima reviewed Sep 7, 2022

View reviewed changes

.gitignore Show resolved Hide resolved

emilpaw force-pushed the setup-typescript branch from 2fe6184 to 99f7512 Compare September 11, 2022 13:55

github-actions bot added the theme: cli label Sep 12, 2022

mshima reviewed Sep 12, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

tsconfig.json Outdated Show resolved Hide resolved

tsconfig.json Outdated Show resolved Hide resolved

mshima reviewed Sep 12, 2022

View reviewed changes

tsconfig.json Show resolved Hide resolved

mshima reviewed Sep 12, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

mshima reviewed Sep 14, 2022

View reviewed changes

cli/environment-builder.js Outdated Show resolved Hide resolved

emilpaw mentioned this pull request Sep 15, 2022

Move source files to src #19750

Closed

6 tasks

emilpaw force-pushed the setup-typescript branch 2 times, most recently from 6136ea8 to c44818b Compare September 15, 2022 16:51

emilpaw added 8 commits September 15, 2022 18:58

Init tsconfig.json

05e7e81

Initial config of build npm script

a3cd118

Configure TS support in eslint

597161e

Add include field to tsconfig.json

dced0f0

Set allowSyntheticDefaultImports to true

22ba324

Fix quotation in .eslintrc.json

2c44fb8

Add dist folder to .prettierignore

47a244c

Add dist folder to .eslintignore

ba674bb

emilpaw and others added 5 commits September 15, 2022 18:58

Pin dependencies

cd5df79

Set build target to ES2022

2901e4a

Ignore more files in copy-files script

022cf4d

Don't copy files ending with .snap and folders starting with __ (currently __snapshots__ & __workflow) to build output. These are files needed for tests or CI which have no use in the build output.

Add test files to exclude in tsconfig.json

93e604a

Update cli/environment-builder.js

931bc39

emilpaw force-pushed the setup-typescript branch 2 times, most recently from e9640be to 931bc39 Compare September 15, 2022 18:40

Adapt paths in exports to build output

018aad2

mshima reviewed Sep 15, 2022

View reviewed changes

test-integration/scripts/10-install-jhipster.sh Outdated Show resolved Hide resolved

Don't build when installing from package

9da2a7c

emilpaw marked this pull request as ready for review September 16, 2022 08:52

DanielFran requested a review from mshima September 16, 2022 08:55

mshima reviewed Sep 16, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

mshima reviewed Sep 16, 2022

View reviewed changes

emilpaw added 4 commits September 16, 2022 16:15

Build project in prepare npm script

590f9fb

This will build the project when it's installed.

Don't build before installing

76f5592

This should be done by the prepare npm script now.

Remove commented out tsconfig options

0af077b

emilpaw force-pushed the setup-typescript branch from 5263183 to 0af077b Compare September 16, 2022 14:26

mshima approved these changes Sep 16, 2022

View reviewed changes

mshima merged commit 71b7e73 into jhipster:main Sep 16, 2022

emilpaw deleted the setup-typescript branch September 16, 2022 17:06

This was referenced Sep 16, 2022

Fix docker image build #19763

Merged

Move source files to src #19767

Closed

DanielFran added this to the 8.0.0-beta.1 milestone Jun 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set up TypeScript #19559

Set up TypeScript #19559

emilpaw commented Aug 28, 2022 •

edited

Tcharl commented Sep 5, 2022

emilpaw commented Sep 11, 2022

mshima commented Sep 11, 2022

emilpaw commented Sep 11, 2022

mraible commented Sep 11, 2022

mshima commented Sep 11, 2022

mshima left a comment

mshima commented Sep 13, 2022

emilpaw commented Sep 14, 2022

mshima commented Sep 14, 2022 •

edited

emilpaw commented Sep 15, 2022

mshima commented Sep 15, 2022

emilpaw commented Sep 15, 2022 •

edited

mshima Sep 16, 2022

emilpaw Sep 16, 2022

mshima left a comment

mshima Sep 16, 2022

mshima Sep 16, 2022

mshima Sep 16, 2022

emilpaw Sep 16, 2022

Set up TypeScript #19559

Set up TypeScript #19559

Conversation

emilpaw commented Aug 28, 2022 • edited

Tcharl commented Sep 5, 2022

emilpaw commented Sep 11, 2022

mshima commented Sep 11, 2022

emilpaw commented Sep 11, 2022

mraible commented Sep 11, 2022

mshima commented Sep 11, 2022

mshima left a comment

Choose a reason for hiding this comment

mshima commented Sep 13, 2022

emilpaw commented Sep 14, 2022

mshima commented Sep 14, 2022 • edited

emilpaw commented Sep 15, 2022

mshima commented Sep 15, 2022

emilpaw commented Sep 15, 2022 • edited

mshima Sep 16, 2022

Choose a reason for hiding this comment

emilpaw Sep 16, 2022

Choose a reason for hiding this comment

mshima left a comment

Choose a reason for hiding this comment

mshima Sep 16, 2022

Choose a reason for hiding this comment

mshima Sep 16, 2022

Choose a reason for hiding this comment

mshima Sep 16, 2022

Choose a reason for hiding this comment

emilpaw Sep 16, 2022

Choose a reason for hiding this comment

emilpaw commented Aug 28, 2022 •

edited

mshima commented Sep 14, 2022 •

edited

emilpaw commented Sep 15, 2022 •

edited