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 Preliminary Support For Javascript-based Configs #1000

Closed
wants to merge 5 commits into from

Conversation

sghoweri
Copy link
Contributor

@sghoweri sghoweri commented Mar 31, 2019

Closes #999

WIP - adding preliminary support for dynamic JS-based configs.

Todos

  • Wire up cosmic config to allow support for .js and .json based configs with minimal overhead
  • Update existing tests to pass with updated config
  • Troubleshoot config getting overwritten with JSON-like output (have to revert every time atm
  • Finish updating docs / code comments

CC @EvanLovely

Copy link
Member

@EvanLovely EvanLovely left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if we can get backwards compatibility with the .json format in addition to the new .js format.

@@ -36,8 +36,7 @@ module.exports = class PatternLab {
constructor(config) {
// Either use the config we were passed, or load one up from the config file ourselves
this.config =
config ||
fs.readJSONSync(path.resolve(__dirname, '../../patternlab-config.json'));
config || require(path.resolve(__dirname, '../../patternlab-config.js'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we just do this instead and then either .json or .js (or even patternlab-config/index.js) would work? Backwards compatible so previous installs do not error if they have .json and might even help with some of the other problems you've mentioned.

- require(path.resolve(__dirname, '../../patternlab-config.js'))
+ require(path.resolve(__dirname, '../../patternlab-config'))

'../uikit-workshop/views/partials/patternSection.mustache',
patternSectionSubtype:
'../uikit-workshop/views/partials/patternSectionSubtype.mustache',
viewall: '../uikit-workshop/views/viewall.mustache',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we need require.resolve() on all these uikit-workshop paths too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #1225

'../uikit-workshop/views/partials/patternSection.mustache',
patternSectionSubtype:
'../uikit-workshop/views/partials/patternSectionSubtype.mustache',
viewall: '../uikit-workshop/views/viewall.mustache',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moar require.resolve() spots

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #1225

@EvanLovely
Copy link
Member

EvanLovely commented Apr 6, 2019

OK, so it looks like plugins have the ability to "enable" and "disable" themselves by firing off a cli command that alters the patternlab-config.json for you:

enable.js:

yield writeJsonAsync(options.parent.config, config);
spinner.succeed(`⊙ patternlab → Updated config`);

There's one in disable.js too:

yield writeJsonAsync(options.parent.config, config);
spinner.succeed(`⊙ patternlab → Updated config`);

And another one in install.js as well:

yield writeJsonAsync(options.parent.config, config);
spinner.succeed(`⊙ patternlab → Updated config`);

So since options.parent.config at this point is what we passed in via command line flag: patternlab-config.js, this writes a string of JSON to that file....

This is the ultimate low-level function that gets called that overwrites our file:

const writeJsonAsync = (filePath, data) =>
wrapAsync(function*() {
yield fs.outputJSON(filePath, data, { spaces: 2 });
});

@EvanLovely
Copy link
Member

Well, crap 💩 this will be a tough refactor with those in there. Perhaps keeping it json and then passing the string paths we get from it through resolvePkg might work? This function caught my eye:

/**
* @func replaceConfigPaths
* @desc Immutable replace source and public paths in the passed config.
* @param {config} config - The passed Pattern Lab config.
* @param {string} projectDir - The project directory path, defaults to ./
* @param {string} sourceDir - The source root directory path.
* @param {string} publicDir - The public root directory path.
* @param {string} exportDir - The export root directory path.
* @return {config} - Returns a modified config. Original stays unaltered.
*/
function replaceConfigPaths(

Or perhaps we can replace this:

      "patternlabFiles": {
        "general-header": "../uikit-workshop/views/partials/general-header.mustache",
        "general-footer": "../uikit-workshop/views/partials/general-footer.mustache",
        "patternSection": "../uikit-workshop/views/partials/patternSection.mustache",
        "patternSectionSubtype": "../uikit-workshop/views/partials/patternSectionSubtype.mustache",
        "viewall": "../uikit-workshop/views/viewall.mustache"
      },

with this:

      "patternlabFiles": {
        "general-header": "./node_modules/@pattern-lab/uikit-workshop/views/partials/general-header.mustache",
        "general-footer": "./node_modules/@pattern-lab/uikit-workshop/views/partials/general-footer.mustache",
        "patternSection": "./node_modules/@pattern-lab/uikit-workshop/views/partials/patternSection.mustache",
        "patternSectionSubtype": "./node_modules/@pattern-lab/uikit-workshop/views/partials/patternSectionSubtype.mustache",
        "viewall": "./node_modules/@pattern-lab/uikit-workshop/views/viewall.mustache"
      },

I tried that, but then I just now got this weird error and I'm not able to pursue further at the moment:

Pattern Lab Node v3.0.1-alpha.0
Pattern Engine mustache: good to go
Pattern Engine twig-php: good to go
Error: ENOENT: no such file or directory, open '/Users/Evan/dev/pl/pl--node/patternlab-node/packages/edition-twig/node_modules/@pattern-lab/uikit-workshop/node_modules/@pattern-lab/uikit-workshop/views/partials/general-header.mustache'

ERROR: missing an essential file from /Users/Evan/dev/pl/pl--node/patternlab-node/packages/edition-twig/node_modules/@pattern-lab/uikit-workshop[object Object]. Pattern Lab won't work without this file.

Incremental builds enabled.
⊙ patternlab → build: Yay, your Pattern Lab project was successfully built ☺

@EvanLovely
Copy link
Member

OMG! The UI kits resolve those paths already relative to where they are! All this time I thought that ../uikit-workshop was going up and out of the the edition-twig directory and into the uikit-workshop directory but it WASN'T! It was going up and out of the uikit-workshop directory and then right back into that same directory!!!! 😆

This change fixes it!!

      "patternlabFiles": {
        "general-header": "views/partials/general-header.mustache",
        "general-footer": "views/partials/general-footer.mustache",
        "patternSection": "views/partials/patternSection.mustache",
        "patternSectionSubtype": "views/partials/patternSectionSubtype.mustache",
        "viewall": "views/viewall.mustache"
      },

PR up at #1003

@cybtachyon
Copy link

Did we already link in the doc/spec updates PR for this or do I need to go make one to https://github.com/pattern-lab/the-spec/ to update the spec & docs?

@sghoweri
Copy link
Contributor Author

sghoweri commented Apr 16, 2019

Did we already link in the doc/spec updates PR for this or do I need to go make one to https://github.com/pattern-lab/the-spec/ to update the spec & docs?

@cybtachyon Spec? What spec? (Seriously, all I see is rough outline that hasn’t gotten updated in a long time).

IMHO, what we REALLY need is a schema that lives in the same repo as the rest of PL’s code (not a separate repo that’s guaranteed to go out of date). That’s the sort of thing that can get tested against, have documentation generated from it, and can get enforced.

@bradfrost
Copy link
Member

what we REALLY need is a schema that lives in the same repo as the rest of PL’s code (not a separate repo that’s guaranteed to go out of date). That’s the sort of thing that can get tested against, have documentation generated from it, and can get enforced.

@sghoweri would love to talk more about what's required to make that happen!

@stale
Copy link

stale bot commented Aug 1, 2019

It's hard to keep track of everything. This issue has been automatically marked as stale because it has not had recent activity, neither from the team nor the community. It will be closed if no further activity occurs. Please consider adding additional info, volunteering to contribute a fix for this issue, or making a further case that this is important to you, the team, and the project as a whole. Thanks!

@bmuenzenmeyer bmuenzenmeyer added the pinned 📌 Don't let stalebot clean this up label Aug 23, 2019

module.exports = {
engines: {
twig: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

twig engine should not be in core?

Copy link
Member

@bmuenzenmeyer bmuenzenmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am excited for this !

think it'd be a good time to change to .patternlabrc.js? it's the shortest and aligns with recommended config defaults for eslint => .eslintrc.js

is .patternlabrc.js used by the uikit right now?

@sghoweri
Copy link
Contributor Author

is .patternlabrc.js used by the uikit right now?

@bmuenzenmeyer It is!

@bmuenzenmeyer
Copy link
Member

@sghoweri I am likely going to use this heavily as a reference but pivot to a new approach. are you alright with that?

@sghoweri
Copy link
Contributor Author

sghoweri commented Feb 9, 2020

👍🏼

@JosefBredereck
Copy link
Contributor

Closing this for now, trying to implement it in a second approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned 📌 Don't let stalebot clean this up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add .js Pattern Lab Config Support
7 participants