Skip to content
This repository has been archived by the owner on Jul 19, 2021. It is now read-only.

Simplify @shopify/slate-config and refactor where it's used #725

Merged
merged 1 commit into from
Sep 6, 2018

Conversation

t-kelly
Copy link
Contributor

@t-kelly t-kelly commented Sep 5, 2018

The Problem

Configuration in it's current state suffers from the following problems:

Config schemas are gross

Config files in Slate packages are long, arduous object collections comprised of multiple key values. Right out of the gate, this makes it hard to understand configuration and how to add/remove them.

{
  id: 'slateTools',
  items: [
    {
      id: 'webpackCommonExcludes',
      default: ['node_modules', 'assets/static/'],
      description: 'Paths to exclude for all webpack loaders',
      type: 'array',
    }
  ]
}

slate.config.js is complicated

The slate.config.js relies on complex object data structure and weird namespacing which makes it hard to write a config items

module.exports = {
  slateCssVarLoader: {
    cssVarLoaderLiquidPath: ['src/snippets/css-variables.liquid'],
  },
  slateTools: {
    extends: {
      dev: {resolve: {alias}},
      prod: {resolve: {alias}},
    },
  },
};

Configurations items cannot access other configuration items

This makes it really hard to bubble config values across a project and limits the potential of Slates config files. An example would be if we had a path.theme config which established where the root folder of the theme is. It's currently impossible to change that config, and see it propagate to other configurations like where the paths.dist and paths.src folders are. See the paths item in the current config to see what I mean.

Config items are not all unit tested

This introduces room for regressions in future releases, which would really suck for configurable parts of Slate.

A simplified config approach

This PR introduces a simplified configuration approach to Slate. Configuration items in Slate packages are declared in a flat object structure with unique strings as keys:

module.exports = {
  // Returns the root directory of the theme
  'paths.theme': process.cwd(),
}

Configurations items can access other configuration items by setting the value of the configuration item to a function, which receives the configuration instance as an argument:

// Theme node_modules directory
  'paths.theme.nodeModules': (config) =>
    path.join(config.get('paths.theme'), 'node_modules'),

Because we now have a flat object structure, slate.config.js is also simplified. All configurations can be changed by referencing their unique key:

module.exports = {
  'paths.theme.nodeModules': '~/node_modules',
}

Config items can be accessed synchronously in Slate packages by using the get() method:

config.get('paths.theme.nodeModules');

TODO in other PRs

  • Update docs for @shopify/slate-config package
  • Refactor Shopify Starter Theme to use new config structure
  • Add tests to all existing configurations
  • Add documentation for each configuration (Missing slate.config.js documentation #690)

@t-kelly t-kelly self-assigned this Sep 5, 2018
@t-kelly t-kelly force-pushed the refactor-slate-config branch 3 times, most recently from 81106a1 to 19e1cea Compare September 6, 2018 14:41
- Refactor @shopify/slate-config to use new config structure
- Replace all config schemas with new structure
- Replace all usage of config items with new .get() format
- Fix any exisiting tests for config items
@t-kelly t-kelly changed the title Refactor @shopify/slate-config Simplify @shopify/slate-config and refactor where it's used Sep 6, 2018
@t-kelly t-kelly merged commit e32d81c into master Sep 6, 2018
@t-kelly t-kelly deleted the refactor-slate-config branch September 6, 2018 15:15
@jonathanmoore
Copy link
Contributor

Is this a breaking change and does it impact the existing configs we have in place? Will they continue to work as-is or will we need to rewrite them?

@t-kelly
Copy link
Contributor Author

t-kelly commented Sep 6, 2018

@jonathanmoore yes it will be a breaking change -- I will be detailing it when I make the release! (still lots to do before that)

@lock
Copy link

lock bot commented Oct 26, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants