bjoluc / jspsych-builder

A CLI utility to easily develop and package jsPsych experiments
MIT License
46 stars 12 forks source link

Add support for `builder.config.[cm]?js` (a configuration file to override parts of `jspsych-builder`) #27

Closed jmuchovej closed 2 years ago

jmuchovej commented 2 years ago

Right now, there's only Webpack override support, but this could be used for other aspects of jspsych-builder overrides.

I had to specify this as a commonjs module to support runtime imports (not sure why). If there are better approaches that this, let me know. I'm a NodeJS novice. 😅

Makes progress to #21, but could also allow for an easy resolution of #19.

bjoluc commented 2 years ago

Thanks for working on this! Fully agree on mimicking the way Next.js does it. TBH, I thought they were using webpack.config.js when I suggested that file name. Since our config file is not really related to jsPsych itself, what do you think about naming it builder.config.js? jspsych-builder.config.js might be better but is quite long :confused:

Re the port thing: Since just the run command is using it, I'd prefer adding it as a CLI option there, rather than here. Would that work for you too, or would you like to have support for both?

jmuchovej commented 2 years ago

Hmm, I opted for jspsych.config.[cm]js precisely because jspsych-builder.config.[cm]?js is rather long. 😅 But I guess it's not a big deal to name it jspsych-builder.config.[cm]?js.

I also threw the port configuration in there, partly out of a lack of familiarity with yargs and Listr, but also because I also have an incredibly strong preference towards file-based configurations over options.

Up to you, I guess, if it stays in the file. I don't see an issue with adding a CLI option, which has the highest priority, though.

bjoluc commented 2 years ago

But I guess it's not a big deal to name it jspsych-builder.config.[cm]?js.

So you don't think builder.config.js would be an option?

I also threw the port configuration in there, partly out of a lack of familiarity with yargs and Listr, but also because I also have an incredibly strong preference towards file-based configurations over options.

Fully understandable. In general, I'd like to keep the API surface as small as possible (maintenance + documentation), so I'd prefer only one place for the port config. Since this is a CLI app, I'd like to have the port option available in the CLI – much like Next.js does it.

jmuchovej commented 2 years ago

In general, I'd like to keep the API surface as small as possible (maintenance + documentation) ...

Makes sense. No qualms here.

So you don't think builder.config.js would be an option?

Hmm... it seems fine to shorten to builder.config.[cm]?js, but if this config really only handles Webpack modificiations, then maybe it makes sense to stick to name the config file webpack.config.[cm]?js?

bjoluc commented 2 years ago

but if this config really only handles Webpack modificiations, then maybe it makes sense to stick to name the config file webpack.config.[cm]?js?

Seems reasonable. But as you said

Right now, there's only Webpack override support, but this could be used for other aspects of jspsych-builder overrides.

so I'm in favor of leaving the door open here :slightly_smiling_face:

jmuchovej commented 2 years ago

Okay, cool. Then I think this LGTM. 👍

bjoluc commented 2 years ago

I was about to ask for your review :sweat_smile: If there's anything in the readme regarding this that you think can be improved, let me know (or push a fix here directly)!

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 4.2.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: