Skip to content

Conversation

@ashbrener
Copy link

@ashbrener ashbrener commented Jun 4, 2020

Description of what you did:

TypeScript support for Strapi has long been a critical success factor for many in this community.

It is clear why there have been heated discussions about this across several forums, and understood why the idea of retrofitting TypeScript support through the Strapi core make a lot of us nervous, especially those who are not familiar or do not have an appreciation of TypeScript.

With that being said, we do not have to overthink a solution to this and hopefully the approval of this PR will solve 90% of the TypeScript use cases out there.

Personally, I do not need any of the generated code (controllers, models etc) to be ported in TypeScript.
I steer clear of making any manual edits to these files.

Approach

I have endeavored to touch the Core as little as possible.

I do not yet have clarity on how best to approach testing, and will obviously leave documentation until the PR is approved as time is precious.

Issues this PR addresses:

  1. [FR] Support Typescript #630
  2. TypeScript types do not match reality #6141

TypeScript Compilation Options

There are 2 ways to enable TypeScript in a project:

  1. Appending a --typescript flag to the strapi CLI command eg
strapi develop --typescript

This method registers ts-node support under the hood and is usually preferably suited for running in development mode.

  1. Compiling using the TypeScript compiler by running a command such as:
tsc -p ./tsconfig.json  --listEmittedFiles --extendedDiagnostics

This method outputs the transpiled Javascript and source maps alongside their TypeScript parent source file.

Conceptually it is not best practise to use ts-node in production as there is a concern of a slight performance impact.
However in my experience if there is, it is unnoticeable.
Further, I prefer not retaining the transpiled files in my project.

TypeScript Example Project

I have been careful to limit injection of TypeScript specific code in the overall project, and so for now have not made changes to the get started example project.
I have created a new example project to show example usage.

Namely:

API Controller

Bootstrap

Plugin Extension

Hook

Middleware

Core Changes

New Dependencies

I am curious why the Strapi project has no devDependencies in package.json which would be the preferred reference for the TypeScript dependencies.
However I have followed protocol by adding the following to dependencies:
typescript, ts-node and tsconfig-paths

TypeScript file support

I have added support for requiring .ts file extensions (for ts-node) which provides support for the following:

  1. Controllers
  2. Services
  3. Hooks
  4. Middleware
  5. Bootstrap
  6. Plugins and Plugin Extension

CLI

Addition of a new typescript flag

strapi start --typescript

packages/strapi/bin/strapi.js
packages/strapi/lib/commands/start.js

strapi develop --typescript

packages/strapi/bin/strapi.js
packages/strapi/lib/commands/develop.js

Type Declarations

It may not be necessary (or practical) to describe the entire shape of the Strapi object instance.
The previous version left a ton of room for improvement, and I have had good shot at it though it may need expansion, and possibly some reduction as well.

index.d.ts

New Project Generator

Addition of 2 new files to a new base project:

tsconfig.json

Strapi object instance global namespace support

Conclusion

I have implemented a marvelously simple solution here and hoping I get support here.

All comments and contributions welcome, I look forward to helping the community accelerate this process and have it included in the core as quickly as possible.

Signed-off-by: ashbrener <ashley@starlogik.com>
@ashbrener ashbrener changed the title Enable TypeScript support Enables TypeScript Support (instantly) Jun 4, 2020
@codecov
Copy link

codecov bot commented Jun 4, 2020

Codecov Report

Merging #6550 (2c2b062) into master (afde2f2) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6550   +/-   ##
=======================================
  Coverage   20.05%   20.05%           
=======================================
  Files         858      858           
  Lines       12049    12049           
  Branches     1951     1951           
=======================================
  Hits         2416     2416           
  Misses       8062     8062           
  Partials     1571     1571           
Flag Coverage Δ
front 14.67% <ø> (ø)
unit 42.45% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/strapi/lib/load/filepath-to-prop-path.js 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3e078c...2c2b062. Read the comment docs.

@alexandrebodin
Copy link
Member

Hello thank you for your PR.

Here are a few comments:

  • I really don't think strapi should be loading any typescript file at runtime.
    ts-node/register is not to be used for production usage. typescript should be compiled and then run by strapi.

  • Typescript should not be added as a dependency of Strapi but to the generated projects instead.

  • We just need to support loading files from a specific directory so a user can have a src and a dist dir and tell strapi to run the dist dir.

We would just need to update the generators to add a typecript option and add the npm scripts to run strapi develop / start etc.

@ashbrener
Copy link
Author

ashbrener commented Jun 5, 2020

@alexandrebodin thank you for your comments.

As stated in my description above I agree with you that it is not best practise to register ts-node in production. However in my experience this decision varies between developers. Accordingly in my PR ts-node will only be registered IF the--typescript flag is present in the CLI command. Arguably if this is a concern, it may be worth only allowing the --typescript flag on the strapi develop command.

Personally I do not like to manually compile with tsc everytime I run the server in development mode. Equally the watcher will then need to trigger tsc every time a .ts file changes.

I agree it would be better to require typescript and associated dependencies in the generated projects.

Compiling typescript to a dist dir separates (alienates) it from all other code scripts where it could happily coexist. As per my description above if one runs tsc the source is compile in place and sits next to the original TyperScript source file. This is actually similar to making changes to entities in Strapi admin which regenerates .js code files in the project.

@alexandrebodin
Copy link
Member

@alexandrebodin thank you for your comments.

As stated in my description above I agree with you that it is not best practise to register ts-node in production. However in my experience this decision varies between developers. Accordingly in my PR ts-node will only be registered IF the--typescript flag is present in the CLI command. Arguably if this is a concern, it may be worth only allowing the --typescript flag on the strapi develop command.

  • adding --typescript option in the commands is not a good solution. You should generate it once with a ts option and that's it. adding those option on other commands couples Strapi with typescript too strongly.

Personally I do not like to manually compile with tsc everytime I run the server in development mode. Equally the watcher will then need to trigger tsc every time a .ts file changes.

  • The only way to address that issue would be to allow loading ts files dynamically. I would be willing to allow this if we upgrade strapi bootstraping to customize the file loading system like you would do with the webpack config . But loading ts files and requiring ts dependencies in strapi core is not an option.

I agree it would be better to require typescript and associated dependencies in the generated projects.

Compiling typescript to a dist dir separates (alienates) it from all other code scripts where it could happily coexist. As per my description above if one runs tsc the source is compile in place and sits next to the original TyperScript source file. This is actually similar to making changes to entities in Strapi admin which regenerates .js code files in the project.

  • This is a matter of configuration, if strapi is told to load form for example ./app and you compile the app folder in place it will work juste fine it will just load the compiled js files if you prefer

@ashbrener
Copy link
Author

ashbrener commented Jun 5, 2020

adding --typescript option in the commands is not a good solution. You should generate it once with a ts option and that's it. adding those option on other commands couples Strapi with typescript too strongly.

  • This makes sense.

The only way to address that issue would be to allow loading ts files dynamically. I would be willing to allow this if we upgrade strapi bootstraping to customize the file loading system like you would do with the webpack config . But loading ts files and requiring ts dependencies in strapi core is not an option.

  • Yes you could do it like webpack. In fact it maybe a sizable task but you could also structure the entire project in a src / dist model that merges the admin build process. That is strapi build would webpack build the entire project into a dist folder including admin where production can be run from.

  • Another simple way to abstract typescript is with a new strapi-typescript package that when included in the command line enables typescript and any other specific functionality. Example:

NODE_OPTIONS="--require strapi-typescript/register" strapi develop

Alternatively even simpler if we only need to enable TypeScript just include ts-node directly if developer wishes:

NODE_OPTIONS="--require ts-node/register" strapi develop

This is a matter of configuration, if strapi is told to load form for example ./app and you compile the app folder in place it will work juste fine it will just load the compiled js files if you prefer

  • Yes this works well for me, exept for in the config/functions directory which tries to load the .js.map files as well (due to the walk loader function in Strapi core I assume)

@ashbrener
Copy link
Author

@alexandrebodin latest commit has the following changes:

  1. Removed all Typescript references from Strapi core:
  • ts-node and tsconfig-paths registration
  • CLI --typescript flag
  1. Moved typescript, ts-node and tsconfig-paths dependencies from Strapi core to the strapi-generate-new package.

  2. In typescript-example project added ts-node registration to NPM develop script, eg:

NODE_OPTIONS="--require ts-node/register" strapi develop

@alexandrebodin
Copy link
Member

Hello @ashbrener can you cleanup the example app so it only contains one api and one model ? it would be easier to review and we won't have to maintain two duplicates of the same app

@ashbrener
Copy link
Author

ashbrener commented Jun 9, 2020

Hi @alexandrebodin are you happy with a separate example app or could it be preferable to simply include a TypeScript API controller in the getstarted project ?

@alexandrebodin
Copy link
Member

Separate examples are fine and easier to read for the community :) If you can just make it a bit simpler so we don't have to maintain it as we update the main one it would be great

@ashbrener
Copy link
Author

👍 will do @alexandrebodin

Signed-off-by: ashbrener <ashley@starlogik.com>
@ashbrener
Copy link
Author

@alexandrebodin
Copy link
Member

Hey @ashbrener sorry I don't know what happened but your branch has a lot of commits that should be here. did you pull develop or sth ?

@ashbrener
Copy link
Author

Pulled from master. Revert ?

Signed-off-by: ashbrener <ashley@starlogik.com>
Signed-off-by: ashbrener <ashley@starlogik.com>
@ashbrener
Copy link
Author

@alexandrebodin I have reset and reapplied some of the mods.

@santoshyadavdev

This comment has been minimized.

@ejferg

This comment has been minimized.

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loading files with ts extensions should be only enabled when we run with a specific TS option (can be an env var for ex) to avoid loading ts files when running a compiled app. I would also like to see a way to no use ts-node and build in two different folders as some users to that instead.

@ashbrener
Copy link
Author

ashbrener commented Jun 16, 2020

loading files with ts extensions should be only enabled when we run with a specific TS option (can be an env var for ex) to avoid loading ts files when running a compiled app.

We could detect if ts-node is running and augment file extension options if applicable.
Can do this by setting the following (eg in strapi/lib/index.js)

// Typescript
process.isTypescriptEnabled = !!process[Symbol.for('ts-node.register.instance')];

Then, in areas where we load files (eg in filepath-to-prop-path.js) we can augment the file extension regular expression.

So this:

const prop = cleanPath
  .replace(/(\.settings|\.json|\.js)/g, '')
  .toLowerCase()
  .split('/')
  .map(p => _.trimStart(p, '.'))
  .join('.')
  .split('.');

... becomes:

const pathRegex = new RegExp(`(\.settings|\.json|\.js${ process.isTypescriptEnabled ? '|\.ts' : '' })`, 'g');

const prop = cleanPath
  .replace(pathRegex, '')
  .toLowerCase()
  .split('/')
  .map(p => _.trimStart(p, '.'))
  .join('.')
  .split('.');

The downside of course is that if .ts files are compiled in the same location, both versions will be loaded.

I would also like to see a way to no use ts-node and build in two different folders as some users to that instead.

You would then be further abstracting the Strapi file system for api controllers, middleware, hooks, plugins, extensions and so on. I would strongly recommend against.

A more popular approach would be to (webpack) compile the entire application and merge it with the admin site build into a single dist output folder. This is obviously a much more complex modification to the entire system.

Personally I have created custom build tasks with NX that compile and copy all Strapi project files from src directory to dist.

@ashbrener
Copy link
Author

@alexandrebodin what do we still need to do to get this across the line.

@Busin-Riccardo

This comment has been minimized.

Signed-off-by: ashbrener <ashley@starlogik.com>
@alexandrebodin
Copy link
Member

alexandrebodin commented Jul 15, 2020

Hi @ashbrener

I'm not sure I understood your comment on different folders as you are mentionning at the end that you do build your code into a dist folder ?
Compiling with webpack or using tsc to compile files independently doesn't change the result of having a dist folder 🤔 I must have missed sth in your explanation.

Right now I'm still not happy with seeing .ts everywhere in the loading. system... I think we should change this somehow so it is cleaner. A ts user could add the 'ts' extension to the config and update the yarn develop command to load the extension.

Maybe sth like

./config/server.js

module.exports = () => ({
  loader: {
    extensions: ['js', 'json', 'ts']
  }
})

Then you can enable it in dev or prod or whatever and just add the ts-register to the bootstrap file or in the npm scripts 🤔
this would give flexibility to only load js files in prod if the ts files are compiled in place.

What do you think @ashbrener ?

@alexandrebodin alexandrebodin requested a review from Convly July 16, 2020 07:43
@alexandrebodin alexandrebodin added source: core:strapi Source is core/strapi package issue: feature request Issue suggesting a new feature labels Jul 16, 2020
@ashbrener
Copy link
Author

Hi @alexandrebodin, to clarify I have incorporated our Strapi project as 1 of many apps inside a broader Monorepo project constructed using Nx by @Nrwl. The Nx framework uses a series of "builders" to compile all monorepo apps to a single dist directory.
In fact a group of us are working on an awesome custom Strapi builder for Nx which should prove very useful for many.

As you so astutely mentioned earlier in the the thread, one ideally needs to compile the Typescript to an output folder. So we achieve this in our build workflow by compiling (transpile all .ts files, and copy the remainder) to the monorepo dist directory.
With this being said, I still believe that the strapi build command should not only produce the admin client, but should rollup a compiled version of the entire Strapi server as well. Obviously this is challenging with Webpack given that the internal nature of Strapi core uses the file system to persist the data models.

Great idea being able to configure allowable file extensions, that would work well.

@ashbrener
Copy link
Author

@alexandrebodin where to from here ?

@ashbrener
Copy link
Author

@alexandrebodin where to from here ?

Guys we would really like to help you and the community get this over the line.

Kindly guide us.

@derrickmehaffy
Copy link
Member

@ashbrener Alex is currently out on vacation and we have been quite busy at the moment. We will let you know if we need anything else.

Thank you.

@alexandrebodin
Copy link
Member

Hi @ashbrener,

I think that implementing that extensions option would be the way to move forward. If we had that you will be able to load stuff however you want and based on the env too so making it possible to implement the scenarios we mentionned.

With that we could have a good enough support for now. And start thinking at what is next for the next major versions

@Erim32
Copy link
Contributor

Erim32 commented Sep 28, 2020

Hi @ashbrener any update about this feature ?

@ashbrener
Copy link
Author

I will have a look at extensions in due course when we have some s p a c e.

In the interim, anyone who wants to have a crack at it ... feel free :)

@alexandrebodin
Copy link
Member

Hey @ashbrener Nice to see you back here :D FYI @ascension did try some of this implementation but has be inactivte :) You can check this PR here #7803.

@derrickmehaffy
Copy link
Member

This pull request has been mentioned on Strapi Community. There might be relevant details there:

https://forum.strapi.io/t/strapi-integration-with-other-typescript-aplications-in-a-monorepo/1104/2

@strapi-cla
Copy link

strapi-cla commented Nov 30, 2020

CLA assistant check
All committers have signed the CLA.

@alexandrebodin
Copy link
Member

Closing in favor of #9076.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

issue: feature request Issue suggesting a new feature source: core:strapi Source is core/strapi package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants