Feature/optional config file

[IMax153]

This pull request is quite large in its scope and aims to provide a flexible and extensible system for configuring docs-ts, while still allowing the tool to be zero-configuration by default.

The most notable additions to the codebase with this pull request is the ability to optionally specify a docs-ts.json configuration file in the root directory of a project. The available configuration settings and their defaults can be viewed in the updated README for the repository.

@gcanti - I completely understand if you would like to forego the changes made in this pull request as I know that this tool was primarily built for your own use. If you are open to the pull request, I completely open to suggestions for changes. In any case, it was a lot of fun to work on these updates and I learned a lot along the way!

Finally, I am leaving this as a draft for now because I still need to rework all of the tests and add new test files for those modules that are not currently included in the test suite.

Let me know what you think!

Closes #18.

[samhh]

I think an enforceSince or enforceVersion option could be added that defaults to true? I could imagine some folks using this for applications in which there's not really any concept of a "version".

[IMax153]

@samhh - I will be happy to add an enforceVersion option once @gcanti reviews this PR for relevance!

[gillchristian]

@IMax153 awesome work! 🎉 Learned a lot view reviewing (more like skimming) your PR.

I've been working on a fork of this project (https://github.com/gillchristian/docs-ts-extra) with similar goals as your PR. Went for the for mainly because I assumed @gcanti wouldn't want to add fundamental changes such as support for configuration.

My implementation was much simpler, just added the minimum changes to for strict mode (ie. enforce since tag behavior on/off), src and out dirs, and React support (ie. .tsx files). And I'd like to also add support for different types of templates (eg. at work we use MkDocs), a check mode (like prettier's --check), monorepo support, fix some bugs in the generated type signatures.

Will you publish as a fork if @gcanti prefers not to include these changes? Either way I'm willing to contribute.

[gcanti]

aims to provide a flexible and extensible system for configuring docs-ts, while still allowing the tool to be zero-configuration by default.

Awesome, thanks @IMax153

[gillchristian]

Still getting the same any types. I'll see if I can reproduce on a public repo later today.

[IMax153]

Still getting the same any types. I'll see if I can reproduce on a public repo later today.

As always, thank you for your review. Apologies for the continued issues with the type signatures.

I just ran docs generation on another personal project I have been developing and can see the any types as well. It looks like this is only occurring for type signatures that require types defined in external (i.e. installed) modules. But it only happens in certain circumstances (primarily when defining a Constant).

Probably still an issue with ts-morph. Will continue investigating.

[gillchristian]

No worries. Happy to help

[gillchristian]

@IMax153 is there anything I could help with? We have a hackathon at work Thursday and Friday and I plan to work on docs-ts related stuff so I could contribute to this PR as well.

[IMax153]

Hey @gillchristian! Apologies for going MIA - I have been extremely busy with work lately and haven't gotten the chance to work on the fixes that are needed.

Long story short - the reason that we are getting any and {} in the types returned by ts-morph is because I switched the Project configuration to use an in-memory file system (since we are already obtaining the content of all files before constructing the Project instance).

However, when using an InMemoryFileSystemHost it seems as though ts-morph loses the ability to gather certain types from external modules that are imported. What weirder is that ts-morph will correctly gather the types from external modules for user-defined types (e.g. interfaces) but for other user-defined items in a file (e.g. constants, functions) ts-morph does not output the types.

I think the fix is to revert ts-morph to its default behavior. The only problem is that then our tests break since ts-morph is expecting an actual file system to parse when the tests are running.

The solution is probably to provide the ts-morph Project as a dependency to the program. However, this entails specifying both the Project and how files are to be added to a project since we would be calling different methods for adding files to an in-memory file system (for testing) and an actual file system.

The interface for what would need to be added to the program's environment would be something like the following:

import type { Reader } from 'fp-ts'
import * as ast from 'ts-morph'

import * as FS from './FileSystem'

export interface Project {
  readonly project: ast.Project
  readonly addFileToProject: (file: FS.File) => Reader<ast.Project, void>
}

I am curious as to your thoughts! I should have some time next week to start working on this again, but if you want to take a crack at it please feel free! I welcome the help!

[IMax153]

@gillchristian - finally found some time to implement what I had been thinking about regarding the Parser module. Would you mind taking another look and letting me know your thoughts?

[gillchristian]

Apologies for going MIA - I have been extremely busy with work lately and haven't gotten the chance to work on the fixes that are needed.

No worries at all.

I am curious as to your thoughts! I should have some time next week to start working on this again, but if you want to take a crack at it please feel free! I welcome the help!

😅 I ended up working on something different on the hackathon, although related to docs-ts https://github.com/gillchristian/docs-ts-mkdocs

I like the implementation you went for, being able to use real file system and yet supporting the in memory one for tests is the best of both worlds.

I tested it on the project I shared before and now all the types are generated as expected.

One thing I found while testing is that Example.run fails with 'Type checking error' when ts-node path is not found. This might be a yarn problem, I installed docs-ts globally:

$ pwd
/Users/gillchristian/dev/docs-ts
$ yarn global add file:$(pwd)

And looks like that didn't make ts-node available 🤔

Maybe the call to spawnSync could be wrapped in tryCatch. I could add that. Wdyt?

Besides that, what do you think is missing in the PR? I can spend some time this week doing a code review.

[IMax153]

😅 I ended up working on something different on the hackathon, although related to docs-ts https://github.com/gillchristian/docs-ts-mkdocs

No worries at all! I checked out the project and its pretty cool stuff!

One thing I found while testing is that Example.run fails with 'Type checking error' when ts-node path is not found. This might be a yarn problem, I installed docs-ts globally:

Very strange - I wonder if this is an issue with the way that you are installing this fork in your repository? Regardless I completely agree that we are not handling the spawning and closing of the child process in the safest of manners.

Maybe the call to spawnSync could be wrapped in tryCatch. I could add that. Wdyt?

What do you think of the re-implementation of Example.run (also shown below)? It handles piping any errors reported by the stderr output of the child process to the parent process for any case where the returned status code is not 0.

export const run = (command: string, executablePath: string): TE.TaskEither<string, void> =>
  pipe(
    TE.fromEither(E.tryCatch(() => spawnSync(command, [executablePath], { stdio: 'pipe', encoding: 'utf8' }), String)),
    TE.chain(({ status, stderr }) => (status === 0 ? TE.right(undefined) : TE.left(stderr)))
  )

Besides that, what do you think is missing in the PR? I can spend some time this week doing a code review.

I don't really think anything else is missing at this point. We have added the features that we set out to, and we improved the test suite to give us more confidence when we make future changes.

I would say you can go ahead and proceed with a code review! 🚀

[gillchristian]

What do you think of the re-implementation of Example.run

Looks good 👌

I would say you can go ahead and proceed with a code review! 🚀

Will do that then 🚀

[IMax153]

Hey @gillchristian! Just wanted to see if there was anything else I could do to help get this PR over the finish line. I have a few projects in the pipeline with Internal directories that could make use of the new config options.

Let me know!

[gillchristian]

Sorry for the delay 😬

I've tried it on some projects I use and all seems fine. And code wise it looks good. 🚀

I'll summon @gcanti for the approval

[gillchristian]

LGTM!

[IMax153]

Thanks @gillchristian! @gcanti - let us know if you would like us to address anything else before merging

[gcanti]

Thanks @IMax153 @gillchristian LGTM, awesome work.

I'm eager to try it out on fp-ts.

[gcanti]

@IMax153 There's a test which is failing locally, I temporary disabled code coverage for '/src/Core.ts' in jest.config.js to see the results at https://gcanti.github.io/docs-ts/

[IMax153]

Hmm - sorry about that @gcanti. We did have quite a difficult time getting the tests to work properly due to problems with ‘ts-morph’, so I anticipate the issues are originating from there.

I will take a look later today and let you know!

[gcanti]

@IMax153 no problem, if it helps I get the following error if I don't skip the test

Core › main › docs-ts.json › should only cause configurable properties to be overwritten if _config.yml exists

    TypeError: Cannot read property 'includes' of undefined

      239 |           assert.strictEqual(result, undefined)
      240 |           assert.strictEqual(
    > 241 |             fileSystemState['/home/maxbrown/projects/docs-ts/docs/_config.yml'].includes(
          |                                                                                 ^
      242 |               'additional_config_param: true'
      243 |             ),
      244 |             true

      at test/Core.ts:241:81
      at node_modules/fp-ts/lib/Either.js:245:75
      at Object.pipe (node_modules/fp-ts/lib/function.js:188:20)
      at assertRight (test/utils.ts:16:3)
      at test/Core.ts:238:9
      at step (test/Core.ts:44:23)
      at Object.next (test/Core.ts:25:53)
      at fulfilled (test/Core.ts:16:58)

[IMax153]

@gcanti - I see the problem. Not an issue with ‘ts-morph’, just an issue with my brain 🤣.

I hard coded the file path using my system file path when I should have used process.cwd().

If /home/maxbrown/projects/docs-ts/docs/_config.yml is replaced with join(process.cwd(), ‘docs/_config.yml’) the test should pass.

I’ll push an update to the branch once I am back at a computer.

[gcanti]

If /home/maxbrown/projects/docs-ts/docs/_config.yml is replaced with join(process.cwd(), ‘docs/_config.yml’) the test should pass.

Done, I confirm, the test now passes, thank you!

[gcanti]

@IMax153 running v0.6 against fp-ts is ok, the only change is nav_order which now starts from 0 instead of 1. It doesn't seem to cause problems but I would change this line

from

RTE.bind('content', () => RTE.right(printModule(module, order)))

to

RTE.bind('content', () => RTE.right(printModule(module, order + 1)))

in order to get the same output, what do you think?

[IMax153]

@gcanti - LGTM!

I would rather keep the output as consistent as possible with what is currently being output in v0.5, so this change looks great!