feat: config file by zoehneto · Pull Request #55 · mockdeep/typewiz

zoehneto · 2018-04-01T18:30:07Z

Implements #31

Implement configuration parsing and validation against schema
Implement helpers which generate the config for instrument and applyTypes
Add support to typewiz-webpack
Add support to typewiz-node

MadaraUchiha

Just a few comments, nothing huge.

MadaraUchiha · 2018-04-01T18:35:49Z

src/configuration-parser.spec.ts

+import { ConfigurationParser } from './configuration-parser';
+
+describe('ConfigurationParser', () => {
+    it('should throw an error if given non-existing typewiz.json file', () => {


I'm not sure that it should explicitly throw an error when given a nonexistent file.

When there's no configuration, we should probably use some sane defaults.

Makes sense, I'll default to an empty config (since this is allowed as well with a config file specified and the defaults are handled by the code using the config (instrument.ts, apply-types.ts and compiler-helper.ts)).

MadaraUchiha · 2018-04-01T18:40:57Z

src/configuration-parser.ts

+    private typewizConfig: any;
+
+    public parse(configurationPath: string) {
+        const typewizConfigSchema = JSON.parse(fs.readFileSync('src/typewiz.json', { encoding: 'utf8' }));


While we do have sync IO in other places, it's generally where it's totally guaranteed that it's in the first tick (i.e. it's a CLI, or it's at the very start of a program running).

There's no such guarantee here, it might be better to use util.promisify() on fs.readFile and async/await.

Right now ci runs against node 6.4.0 which doesn't have util.promisify() (async await should be transpiled by typescript). Is support for node 6 important or can we use util.promisify() (and then remove node 6 from ci since it wont work anymore.

@urish Thoughts? Node 8.9.0 is the LTS, how far back do we want to support node versions?

MadaraUchiha · 2018-04-01T18:43:04Z

src/configuration-parser.ts

+
+        let typewizConfigString;
+        try {
+            typewizConfigString = fs.readFileSync(path.resolve(configurationPath), { encoding: 'utf8' });


Same as the last comment, sync IO is bad in low level APIs :(

MadaraUchiha · 2018-04-01T18:44:04Z

Very nice! I've reviewed the changes so far. Huzzah 🎉

zoehneto · 2018-04-02T13:48:01Z

@urish I have made the readFile calls asynchronous using util.promisify which breaks node 6 (see review above). If that is a problem I can add the polyfill, otherwise I would remove node 6 from .travis.yml.

MadaraUchiha · 2018-04-02T13:50:23Z

src/configuration-parser.ts

-    public parse(configurationPath: string) {
-        const typewizConfigSchema = JSON.parse(fs.readFileSync('src/typewiz.json', { encoding: 'utf8' }));
+    public async parse(configurationPath: string): Promise<void> {
+        const readFileAsync = util.promisify(fs.readFile);


You should probably do this outside, promisifying is expensive and we shouldn't do it every time parse() is called ;)

I can store it in the class but that leads to issues getting the type right: in the node types readFile is actually 4 different functions all overloading the same name (because the return type changes based on whether you specify an encoding etc.). Do you know how to write that type without having to cast it when assigning it, using it and also not using any?

All I meant was that you move const readFileAsync = ... outside the class, so that the promisification only happens once. There shouldn't be any new typing problems (literally cut and paste line 10 to above the class ... line).

Ah, of course :) I just made the change.

coveralls · 2018-04-02T15:26:51Z

Coverage increased (+4.005%) to 90.309% when pulling 9bb3152 on zoehneto:config-file into d9b532c on urish:master.

coveralls · 2018-04-02T15:26:51Z

Coverage increased (+0.7%) to 86.957% when pulling abd9aee on zoehneto:config-file into 5b9c3d1 on urish:master.

zoehneto · 2018-04-02T17:31:47Z

I added support for the config file to typewiz-webpack, usage:

use: [ 'ts-loader',
    {
        loader: 'typewiz-webpack',
        options: {
            typewizConfig: path.resolve(__dirname, "../typewiz.json")
        }
    }],

This only supports the instrumentation though, since typewiz-webpack doesn't apply types (it creates a json file with all gathered type information.

urish · 2018-04-03T08:01:04Z

@zoehneto I think we can drop node 6 support from the core library, and then use the polyfill just inside typewiz-node to provide a smoother experience there. WDYT?

zoehneto · 2018-04-03T08:42:54Z

According to the docs of the polyfill patching the util module from typewiz-node should work but we definitely have to test that. Do you want to do the same thing with typewiz-webpack?

zoehneto · 2018-04-03T11:38:13Z

I just implemented config support for typewiz-node (node 6 polyfill not yet included). It might be necessary to add a function to recursively search for typewiz.json in parent folders to improve mocha handling (since we can't specify a path for mocha) but I can't say since I don't use it and have no way of testing it.

urish · 2018-04-03T23:21:54Z

@zoehneto migrating to monorepo (#56) shifted some files around - basically, everything that was under src is now in packages/typewiz-core/src. If you need any help with rebasing (now or after the branch is ready), please ping here :-)

zoehneto · 2018-04-04T08:01:42Z

@urish I just did the rebase. I also changed the dependency for typewiz-node and typewiz-webpack to typewiz-core instead of typewiz.

…ode 6.4.0 in typewiz-node and typewiz-webpack through a polyfill

zoehneto · 2018-04-04T08:28:51Z

@urish I just added the polyfill to typewiz-node and typewiz-webpack and updated the node version requirement for typewiz-core. That means this PR is ready for review, I'll add the documentation once that is done. Please also take a look at the open tasks in the PR description since most of those can only be performed once this PR is merged.

urish · 2018-04-05T23:54:53Z

packages/typewiz-node/src/typewiz-node.ts

-register();
+(async () => {
+    if (process.argv.length > 3) {
+        await register({ typewizConfig: process.argv[2] });


I think it will make more sense to search for the config file using the same heuristic ts-node uses for finding tsconfig.json, and if we want, we can also allow the user to override this using an optional command line argument (i.e. typewiz-node --typewiz-config filename.json my-awesome-script.ts).

Otherwise, the current behavior can be somewhat confusing - if you run typewiz-node my-awesome-script.ts the script will execute, but then if you try to pass some parameter to the script, e.g. typewiz-node my-awesome-script.ts --kittens, it will treat the script as the configuration file, and you will get an error.

WDYT?

I think there are two things to consider here:

typewiz.json resolution: As mentioned above we could put in a function which recursively searches for typewiz.json in parent directories. I think that certainly makes sense, especially for mocha integration where we can't specify a config file. ts-node uses a typescript function for that which takes the config name as a parameter, so we should be able to use the same logic

Argument parsing: Using a proper command line argument parser would certainly make this more robust and nicer to use. The only question for me is how do we want to handle ts-node? Currently you can just parse any ts-node argument to typewiz-node and those arguments will be passed along to ts-node. Do we want to maintain this feature (perhaps by stripping all typewiz config options from process.argv)?

I see the typescript function you mentioned is exported, is it a part of the public API? can we just use it?

Regarding argument parsing, I think that stripping all typewiz config options from process.argv will do the trick. But we can skip this for now, remove the extra argument from typewiz-node and just stick with searching typewiz.json as suggested above, so we can get the PR merged sooner.

The function is exported from the main entry point for typescript so I assume it is part of the public api.

I just added a configuration finder based on that function, it is now used by typewiz-webpack and typewiz-node. I also removed the optional argument from typewiz-node for now.

urish · 2018-04-08T21:51:22Z

packages/typewiz-core/src/configuration-parser.ts

+
+    public async parse(configurationPath: string): Promise<void> {
+        const typewizConfigSchema = JSON.parse(
+            await readFileAsync(path.join(__dirname, 'typewiz.json'), { encoding: 'utf8' }),


Since the schema is not going to change in runtime, why not read it the the beginning of the file using require('./typewiz.json') ? This will also eliminate the need for JSON.parse

Thanks 👍

urish · 2018-04-08T21:53:19Z

packages/typewiz-core/src/configuration-parser.ts

+
+        let typewizConfigString;
+        try {
+            typewizConfigString = await readFileAsync(path.resolve(configurationPath), { encoding: 'utf8' });


How about creating an internal parseConfig() function that will take the config as an object? then we could provide parse() and parseSync() as wrappers, and use parseSync() in typewiz-core/register, so we keep the node registration API sync and simple.

Which file do you mean with typewiz-core/register (I moved typewiz-core/node-register to typewiz-node/node-register since that is the place it is used). In general the way the configuration parser is currently used in typewiz-webpack and typewiz-node it would only make sense to provide a synchronous alternative if it was used for everything (which @MadaraUchiha explicitly didn't want in the first review).

My bad - I meant the register() function exported by typewiz-node. The reason we need that one to be sync is for using with mocha. You use it like:

mocha -r typewiz-node/dist/register src/**/test.ts

If this would be done asynchronously, there is a race condition where mocha might start requiring .ts files before we have registered.

Makes sense. But that does mean we have to use the synchronous parsing everywhere in typewiz-node (index.ts and node-register.ts).

That sounds okay to me. ts-node and babel both have synchronous register methods, and as far as I can tell, they also read configuration files.

urish · 2018-04-08T21:54:06Z

Thanks! Two more comments and I think we are ready to go :)

urish · 2018-04-09T09:06:49Z

Lovely, thank you so much! Merge time :)

zoehneto · 2018-04-09T09:13:29Z

Thanks :) What do you want to do about the open tasks still left here (maybe put them in the original issue)?

urish · 2018-04-09T09:41:16Z

Sounds good! Will you be willing to tackle them as well?

zoehneto · 2018-04-09T09:46:54Z

Certainly Schema Store, Documentation and updating the webpack demo (once the new version is relesed). Webpack type applying with config is probably a task to be tackled as part of the cli and correcting the dependency version for typewiz-core would probably best be done by you during publish.

urish · 2018-04-09T10:58:53Z

Excellent

zoehneto · 2018-04-09T13:39:49Z

I have moved the remaining tasks to issue #31

MadaraUchiha suggested changes Apr 1, 2018

View reviewed changes

MadaraUchiha suggested changes Apr 2, 2018

View reviewed changes

zoehneto added 10 commits April 4, 2018 09:41

Implement config file parsing and schema based validation

0ddbe38

Don't throw an exception if the config file doesn't exist

871b84f

Read files asynchronously

b4865ce

Move util.promisify outside of the ConfigurationParser

3cc8631

Add getters for options

16457cc

Prepare configuration parser for external use

4beffc9

Use typewiz config in typewiz-webpack loader

f333b87

Only parse configuration once in loader

26b0ac7

Implement configuration support for typewiz-node

7c636de

Mono repo fixes

74b460d

zoehneto force-pushed the config-file branch from 79492c3 to 74b460d Compare April 4, 2018 07:59

Require node 8 for typewiz-core, remove node 6.4.0 from ci, support n…

be344ed

…ode 6.4.0 in typewiz-node and typewiz-webpack through a polyfill

urish reviewed Apr 5, 2018

View reviewed changes

zoehneto added 3 commits April 8, 2018 13:52

Remove typewiz.json argument from typewiz-node

55ff943

Implement configuration finder

df42286

Resolve compiler options relative to typewiz.json

1f10f8c

zoehneto force-pushed the config-file branch from 1897b5d to 1f10f8c Compare April 8, 2018 13:22

urish reviewed Apr 8, 2018

View reviewed changes

zoehneto added 3 commits April 9, 2018 08:42

Load typewiz.json schema with require

be0a02c

Add parseSync method to configuration parser and use it in typewiz-node

507f1ac

Update docs for typewiz-webpack

9bb3152

urish merged commit d76e5dd into mockdeep:master Apr 9, 2018

zoehneto changed the title ~~feat: config file [WIP]~~ feat: config file Apr 9, 2018

Conversation

zoehneto commented Apr 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MadaraUchiha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadaraUchiha commented Apr 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zoehneto commented Apr 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadaraUchiha Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 2, 2018

Uh oh!

zoehneto commented Apr 2, 2018

Uh oh!

urish commented Apr 3, 2018

Uh oh!

zoehneto commented Apr 3, 2018

Uh oh!

zoehneto commented Apr 3, 2018

Uh oh!

urish commented Apr 3, 2018

Uh oh!

zoehneto commented Apr 4, 2018

Uh oh!

zoehneto commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

urish commented Apr 8, 2018

zoehneto commented Apr 1, 2018 •

edited

Loading

MadaraUchiha commented Apr 1, 2018 •

edited

Loading

MadaraUchiha Apr 2, 2018 •

edited

Loading

coveralls commented Apr 2, 2018 •

edited

Loading