Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to the root config to include a projectConfigFilePath #21

Open
wants to merge 1 commit into
base: master
from

Conversation

@sharmilajesupaul
Copy link
Collaborator

commented Aug 7, 2019

Adds a projectConfigFilePath field to the top level package configuration and a name field on project configs.

The projectConfigFilePath option takes a glob string value to represent the project config file path.

If there is no name field in the project configuration, we assume that the project's name is the name of the parent directory of the config file. We also assume that the new project configurations follow the same structure, and have a top level "react-component-variations" property.

This means, for a configuration like this:

// ./package.json
{
  "react-component-variations": {
    "projectConfigFilePath": "*/frontend/*/project.json",
    "sync": [ ],
    "extras": { }
  }
}

RCV will now look at all the files that match the glob pattern: */frontend/*/project.json and construct the projects field in the configuration object based on the "react-component-variations" field in each project config.

There is a fixture file included in this PR with the file tree:

tree test/fixtures/fun-monorepo/
test/fixtures/fun-monorepo/
├── projectA
│   └── project.json
├── projectB
│   └── project.json
└── projectC
    └── project.json

3 directories, 3 files

If our glob field matches these project.json files, we the configuration output from this new method should look like this:

      { projectConfigPath: '*/fixtures/fun-monorepo/*/project.json',
        projects: 
         { projectA: 
            { componentsRoot: './app/assets/javascripts/foo/team/',
              components: './**/*.{jsx,tsx}',
              variationsRoot: './app/assets/javascripts/foo/examples/',
              variations: './**/*VariationProvider.{jsx,tsx}' },
           projectB: 
            { componentsRoot: './app/assets/javascripts/foo/team/',
              components: './**/*.{jsx,tsx}',
              variationsRoot: './app/assets/javascripts/foo/examples/',
              variations: './**/*VariationProvider.{jsx,tsx}' },
           projectC: 
            { componentsRoot: './app/assets/javascripts/foo/team/',
              components: './**/*.{jsx,tsx}',
              variationsRoot: './app/assets/javascripts/foo/examples/',
              variations: './**/*VariationProvider.{jsx,tsx}' } } }

Since the output config matches the original root schema, we can split out the per project configurations to different files while maintaining all of the original functionality.

TODO:

  • Can we drop support for node v6? it reached its EOL in April
  • Make a prerelease and do a manual test
Add an option to the root config to include a projectConfigFilePath
This option which takes a glob to represent the project config file path and a name property to identify a project name. If there is no name field, we guess the project's name based on the parent directory of the config file. I pulled in es-promisify to make this change since we still running CI against node v6 and I updated glob.

@sharmilajesupaul sharmilajesupaul requested review from ljharb, lencioni and ahuth Aug 7, 2019

jest.mock('glob');

beforeEach(() => {
glob.sync = jest.fn();

This comment has been minimized.

Copy link
@sharmilajesupaul

sharmilajesupaul Aug 7, 2019

Author Collaborator

I couldn't get glob to work normally in my tests, even with very general obvious globs. I was getting a cannot get length of undefined error. :(

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 7, 2019

Collaborator

is there a reason it needs to be mocked at all, since you've got the actual test files committed?

This comment has been minimized.

Copy link
@sharmilajesupaul

sharmilajesupaul Aug 7, 2019

Author Collaborator

I'm able to target the files I need with glob.sync using the glob path on L20 { projectConfigPath: '*/fixtures/fun-monorepo/*/project.json' }. However, when this test runs using Jest, glob.sync throws with the cannot get length of undefined error no matter how general the glob. I think this maybe a bug, any ideas? I can try and make a repro-case.

@lencioni
Copy link
Member

left a comment

Instead of building the collating directly into this project, what do you think about making this project able to look for the configs in a specific variations config file instead of package.json, and allow that to be JavaScript? That would allow you to write the collating logic into the config file itself instead of adding complexity to this project.

@sharmilajesupaul

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 7, 2019

@lencioni I was hoping that we could point this at any configuration file and allow it to house our variation config. That way, if there is a workspace with nested package.jsons or a single project config file. We can use that.
I could also see this working with a single config like. Maybe we could support a .react-component-variations config file that exports the root configuration? and then do the logic of collocating in that? is that what you're suggesting?

@lencioni

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

@sharmilajesupaul yes, that's more or less what I was thinking. What if the only change in this project is to make it look for something like .react-component-variations.js. Then the consuming project would have total control over how that config works.

@sharmilajesupaul

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 7, 2019

@lencioni the way this currently works is the implementation is tied to yargs.pkgConf https://github.com/airbnb/react-component-variations/blob/master/src/helpers/getProjectRootConfig.js#L12
When the exported traversal methods run, they look directly at the package.json instead of at top level defined config file.
https://github.com/airbnb/react-component-variations/blob/master/src/traversal/forEachProjectVariation.js#L21
So I'm not sure how to add this functionality without making a breaking change where we would change the location of the config from package.json to a root configuration file like .react-component-variations. I'm happy to do that, but it would also mean dropping the support for a default package.json config or falling back to it as a secondary option.

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Aug 7, 2019

It's pretty critical to the overarching design of this system that config be only ever static data, never JavaScript.

@@ -33,8 +33,9 @@
"array.prototype.flat": "^1.2.1",
"array.prototype.flatmap": "^1.2.1",
"chalk": "^2.4.2",
"es6-promisify": "^6.0.1",

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 7, 2019

Collaborator

why this, over https://npmjs.com/util.promisify, which is a polyfill for the built-in node functionality?

@@ -16,9 +16,10 @@ export const desc = 'print out component paths for the given project';

export { default as builder } from '../helpers/validateCommand';

export const handler = (config) => {
// This config parameter here is the arguments passed in through yargs.
export const handler = async (config) => {

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 7, 2019

Collaborator

unless we do drop node 6, this means we're transpiling async/await; i'd prefer to avoid that.

This comment has been minimized.

Copy link
@sharmilajesupaul

sharmilajesupaul Aug 7, 2019

Author Collaborator

it would be nice to drop support eventually, since it did reach eol.

This comment has been minimized.

Copy link
@sharmilajesupaul

sharmilajesupaul Aug 7, 2019

Author Collaborator

or can we use a babel transform?

@@ -0,0 +1,46 @@
import path from 'path';
import { readFile } from 'fs';
import { promisify } from 'es6-promisify';

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 7, 2019

Collaborator

util.promisfy?

// and then read each file and create a config object
return Promise.all(globToFiles(projectConfigPath).map(async (file) => {
const projectConfigFileContents = await promisify(readFile)(file);
const config = await JSON.parse(String(projectConfigFileContents))['react-component-variations'];

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 7, 2019

Collaborator

JSON.parse does not return a promise, awaiting it achieves nothing except making the function slower.

Separately, it's been pretty intentional not to hardcode this field name, since it can be overridden on the command line using yargs' --config option.

// Since project names are the top level properties in the projects object
// duplicate keys will overwrite eachother. We throw if we find any duplicate project names.
const projectNameDuplicates = arrayOfProjectConfigs
.map(obj => Object.keys(obj)[0])

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 7, 2019

Collaborator

i'm confused why this is the first key - what if there are multiple?

// that mirrors the root configuration.
return ({
...rootConfig,
projects: arrayOfProjectConfigs.reduce((memo, projectConfig) => ({

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 7, 2019

Collaborator

this seems like a good use case for object.fromentries rather than reduce?

@@ -24,6 +24,11 @@ export default {
extras: { $ref: '#/definitions/extras' },
metadata: { $ref: '#/definitions/metadata' },
sync: { $ref: '#/definitions/sync' },
// Project Name

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 7, 2019

Collaborator

the project name is already encoded in the key that holds this object - why would we want to duplicate it here?

jest.mock('glob');

beforeEach(() => {
glob.sync = jest.fn();

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 7, 2019

Collaborator

is there a reason it needs to be mocked at all, since you've got the actual test files committed?

@lencioni

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

@sharmilajesupaul

So I'm not sure how to add this functionality without making a breaking change where we would change the location of the config from package.json to a root configuration file like .react-component-variations. I'm happy to do that, but it would also mean dropping the support for a default package.json config or falling back to it as a secondary option.

I'm probably missing something, but it looks like there's already an option to specify a path to a config file, right? If so, that could be used. If not, I think making it look for a specifically named config file and then falling back to package.json would be semver minor, and totally fine.

@ljharb

It's pretty critical to the overarching design of this system that config be only ever static data, never JavaScript.

Why?

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Aug 7, 2019

Dynamic configs, jsut like dynamic values, are harder to maintain, to reason about, and to manage. Although there are certainly use cases for dynamic configs, whenever a static config is changed to a dynamic one, there's a loss - a tradeoff.

Regardless, this project has an explicit design goal to only ever have static config.

@lencioni

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Dynamic configs, jsut like dynamic values, are harder to maintain, to reason about, and to manage. Although there are certainly use cases for dynamic configs, whenever a static config is changed to a dynamic one, there's a loss - a tradeoff.

I agree that it might be harder to maintain, but in my experience, I've often switched from a static config to a dynamic one and found it easier to maintain.

this project has an explicit design goal to only ever have static config

Why does this project have this goal?

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Aug 8, 2019

Because of the first part of my comment - in my experience, dynamic configs have always been harder to maintain, and I want to ensure they remain static.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.