Support more config file formats #334

alvis · 2021-10-14T08:54:49Z

This PR changes how the config file is located and parsed in order to support more file format such as crago.config.ts.

Under the hood, it uses cosmiconfig, a reliable and mature config parser, instead of the custom logic to handle the config in different formats.

As a side effect, it also supports cosmiconfig presented as an object in package.json should you prefer it.
If you prefer the old way that cosmiconfig in package.json represents the path to the config file, it works as well.

Fix #235 #290

resolve dilanx#235 dilanx#290

patricklafrance · 2021-10-18T01:29:58Z

packages/craco/lib/config.js

+        loaders: {
+            '.ts': tsLoader,
+        },
+    });


Is there a specific reason why you call again cosmic config instead of using configFilePath ?

Because the config has to be loaded somehow, and they may not be necessarily .json or .js so that you can simply load with require, e.g. .ts or .yaml. cosmiconfig is used here again to load the config file found in paths.js. Hope this make sense :)

Ahhh got it, thanks. Sorry for my ignorance I haven't, use cosmiconfig before.. since you don't specify searchPlaces here, how does cosmiconfig will behave?

Knowing this, I am not sure the code in paths.js to find the config file path is still relevant. Maybe move everything related to the config path in getConfigAsObject ?

It's not ideal I admit. searchPlaces is omitted because the config is going to be loaded from configFilePath so having it here is just redunctant.

When I was implementing it, my first trial was moving everything to under getConfigAsObject, but I realise it will cause a compatibility issue that there is a chance the file path can be specified in package.json which is not picked up by cosmiconfig, so a separate logic is needed to handle that, hence I leave the path finding logic in paths.js, and the loading logic in config.js, even though cosmiconfig can handle both in one call.

I am not sure I follow here. Where would the config be loaded otherwise? When using a custom config path, the path is currently resolved in paths.js but even in your PR the actual config is always loaded in getConfigAsObject.

The problem is that cosmiconfig does recognize the config file path in package.json. It only consider the craco field in package.json as the actual content of the config. So another logic has to be in place to handle that.

Yes, sorry I forgot your first reply which mention that you can't require a TS file. I get it now.

Though maybe paths.js could export a cosmicExplorer instance so it won't have to be configured twice?

It can be done but sharing a cosmiconfig instance would complicate the dependency graph, which I recommend not to. The current structure is fine, paths.js holds functions for finding the config file path, and config.js holds functions that parse the file.

I think the best solution is to move the logic of the config path finding to config.js and leave other path-related exports in paths.js. Then we won't have 2 cosmiconfig instances, and actually much cleaner. Shall I do that?

Yes go ahead. Thanks!

patricklafrance · 2021-10-18T01:34:10Z

Have you installed the extensions? The code should auto format on save to follow the repository code style.

patricklafrance · 2021-10-18T01:34:23Z

Please update the docs to indicate CRACO support TS files.

patricklafrance · 2021-10-18T01:34:38Z

If CRACO support TS file, should CRACO also export typings for it's hooks, plugins and utils?

patricklafrance · 2021-10-18T01:31:53Z

packages/craco/lib/paths.js

        }
+
+        log("Found craco config file at: ", result.filepath);


You could move this log outside of the else to also log if the file config have been located from a path configured in package.json

There is actually already another log outside the else block
https://github.com/gsoft-inc/craco/blob/9e6bd9ed3d4d7861618c7b70048cca9ec1744b12/packages/craco/lib/paths.js#L57

So this line is actually unnecessary.

packages/craco/lib/config.js

packages/craco/lib/paths.js

alvis

New changes committed as suggested.

alvis · 2021-10-18T06:24:09Z

packages/craco/lib/config.js

+        loaders: {
+            '.ts': tsLoader,
+        },
+    });


Because the config has to be loaded somehow, and they may not be necessarily .json or .js so that you can simply load with require, e.g. .ts or .yaml. cosmiconfig is used here again to load the config file found in paths.js. Hope this make sense :)

packages/craco/lib/config.js

alvis · 2021-10-18T06:31:10Z

packages/craco/lib/paths.js

        }
+
+        log("Found craco config file at: ", result.filepath);


There is actually already another log outside the else block
https://github.com/gsoft-inc/craco/blob/9e6bd9ed3d4d7861618c7b70048cca9ec1744b12/packages/craco/lib/paths.js#L57

So this line is actually unnecessary.

alvis · 2021-10-18T07:04:15Z

Please update the docs to indicate CRACO support TS files.

Done

If CRACO support TS file, should CRACO also export typings for it's hooks, plugins and utils?

I'd prefer to leave it for another PR. It'd be nice, but I'm not familiar with the detail of all configs.
Need more time to prepare.

alexasselin008 · 2021-10-18T10:11:36Z

If CRACO support TS file, should CRACO also export typings for it's hooks, plugins and utils?

I’d suggest using the definitelytyped repository for this, since it is easier than converting the entire projet to TS for now

alvis · 2021-10-18T12:32:24Z

It’s not necessary to convert the whole project to ts just to provide a type file. It can by simply be done with an extra index.d.ts file.

patricklafrance · 2021-10-21T13:55:04Z

Merged, thank you @alvis :) Will release soon.

patricklafrance · 2021-10-25T02:00:53Z

Released in https://www.npmjs.com/package/@craco/craco/v/6.4.0!

Codex- · 2021-11-08T22:33:00Z

It’s not necessary to convert the whole project to ts just to provide a type file. It can by simply be done with an extra index.d.ts file.

You can do it incrementally too, where some things are adapted to TS while still using the JS source in other parts.

I'd be happy to start on this in a PR if you'd be interested? I have a lot of experience with JS->TS projects 👀

alvis · 2021-11-09T11:31:03Z

@codex We are definitely interested!

Codex- · 2021-11-09T21:23:28Z

Great! I'll start chipping away at it and throw a PR up when I have a start, and work out some strategy things :)

Codex- · 2021-11-16T00:49:08Z

@alvis I have a few quick questions before I get too far in, where is best to contact you?

alvis · 2021-11-16T07:54:13Z

sure. feel free to DM me on twitter @AlvisHTTang or email alvis at hilbert dot space

Codex- · 2021-11-18T01:03:46Z

Open your DM's on twitter ;)

Also the usage of the typescript config package here uses a no longer maintained package for managing the typescript parsing, which has a few issues and pr's open that resolve bugs I've run into lol so going to fork and publish a fixed version to remedy this.

feat: support more config file formats

9e6bd9e

resolve dilanx#235 dilanx#290

alvis mentioned this pull request Oct 14, 2021

Allow craco config to be written in Typescript #290

Closed

patricklafrance reviewed Oct 18, 2021

View reviewed changes

alvis added 4 commits October 18, 2021 07:36

style: format the code for consistency

5f7603c

docs: state that craco now support config file in typescipt

a170a1c

fix: re-order the config file loading priority

b769a44

fix: remove duplicated log

2072c92

alvis commented Oct 18, 2021

View reviewed changes

refactor: move the config file path finding logic to config.js

21422ef

patricklafrance merged commit 0f59ef0 into dilanx:master Oct 21, 2021

elektronik2k5 mentioned this pull request Oct 29, 2021

Support CRACO's new cosmiconfig and TS configuration formats artisanofcode-archive/storybook-preset-craco#11

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support more config file formats #334

Support more config file formats #334

alvis commented Oct 14, 2021 •

edited

patricklafrance Oct 18, 2021

alvis Oct 18, 2021

patricklafrance Oct 18, 2021 •

edited

alvis Oct 18, 2021

patricklafrance Oct 18, 2021

alvis Oct 19, 2021

patricklafrance Oct 19, 2021

alvis Oct 20, 2021

patricklafrance Oct 20, 2021

alvis Oct 20, 2021

patricklafrance commented Oct 18, 2021

patricklafrance commented Oct 18, 2021

patricklafrance commented Oct 18, 2021

patricklafrance Oct 18, 2021

alvis Oct 18, 2021

alvis left a comment

alvis Oct 18, 2021

alvis Oct 18, 2021

alvis commented Oct 18, 2021

alexasselin008 commented Oct 18, 2021

alvis commented Oct 18, 2021

patricklafrance commented Oct 21, 2021

patricklafrance commented Oct 25, 2021

Codex- commented Nov 8, 2021

alvis commented Nov 9, 2021

Codex- commented Nov 9, 2021

Codex- commented Nov 16, 2021

alvis commented Nov 16, 2021

Codex- commented Nov 18, 2021

Support more config file formats #334

Support more config file formats #334

Conversation

alvis commented Oct 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricklafrance Oct 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricklafrance commented Oct 18, 2021

patricklafrance commented Oct 18, 2021

patricklafrance commented Oct 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvis commented Oct 18, 2021

alexasselin008 commented Oct 18, 2021

alvis commented Oct 18, 2021

patricklafrance commented Oct 21, 2021

patricklafrance commented Oct 25, 2021

Codex- commented Nov 8, 2021

alvis commented Nov 9, 2021

Codex- commented Nov 9, 2021

Codex- commented Nov 16, 2021

alvis commented Nov 16, 2021

Codex- commented Nov 18, 2021

alvis commented Oct 14, 2021 •

edited

patricklafrance Oct 18, 2021 •

edited