RFC: User Config file integration #53

hutchgrant · 2019-04-23T01:22:49Z

Related Issue

Resolves #40

Summary of Changes

reads from greenwood.config.js in root of app directory
current accepted keys:

{
    "source": "src",
    "publicPath": "/",
    "devServer": {
        "port": 1984,
        "host": "localhost"
    },
    "meta": {}
}

integrates with init.js to set the userworkspace path. "source" is prepended with current working directory
integrates with webpack configs
added tests for when using greenwood config file

…ng additional tests

Signed-off-by: HutchGrant <h.g.utchinson@gmail.com>

thescientist13

💥

Nice job! A little beyond the scope, but the changes seem to work well together and you documented them nicely with unit tests, so I think it's OK to set this a base level for this feature. and it's important to reward ambition too, can't always be super sticklers I suppose. 😄 🏆

Just left a couple minor feedback points for review. And for going forward, we'll want to try and track each new config feature we want to add as a new issue.

thescientist13 · 2019-04-24T00:01:42Z

packages/cli/lib/compile.js

@@ -1,29 +1,30 @@
 require('colors');
+const initConfig = require('./config');
 const initContext = require('./init');


probably not for this issue, but maybe we should create an issue to change init.js to context.js?

Yes I agree with that.

thescientist13 · 2019-04-24T00:04:03Z

test/fixtures/greenwood.config.json

@@ -0,0 +1,9 @@
+{


can we set the indentation to 2 here?

yea I don't know why my editor defaults to 4, have to get around to changing that.

thescientist13 · 2019-04-24T00:04:55Z

test/setup.js

-          testApp: path.join(__dirname, 'fixtures', 'mock-app', 'src')
+          testApp: path.join(__dirname, 'fixtures', 'mock-app', 'src'),
+          userCfgPath: path.join(__dirname, 'fixtures', 'greenwood.config.json'),
+          userCfgRootPath: path.join(__dirname, '..', 'greenwood.config.json')


how come we are using this second config file? The names are really close and hard to distinguish. Maybe we just need more fixtures?

My comment here about creating "cases" of tests might be an appropriate justification.

second config is just the hardcoded path to where the config will be copied. Hence: userCfgRootPath

ah, so basically for mocking the user's project root, where their own greenwood.config.json would be located?

thescientist13 · 2019-04-24T00:10:34Z

test/cli.spec.js

@@ -160,4 +160,67 @@ describe('building greenwood with error handling for app and page templates', ()
    await fs.remove(CONTEXT.scratchDir);
  });

+});
+
+describe('building greenwood with user provided config file', () => {


if the intent is to also test with a nested directory output, lets add it to the describe description, otherwise I think we can drop those additional tests for nested pages since they are already covered above, and if our architecture is sound, anything that breaks nested directories here should in theory also break it up there, which is a better logical block to solve it in.

In essence, we can try and have tests that take advantage of other tests already testing for specific outputs, so tests can have minimal overlap while still breaking each other if something goes wrong when making a change.

They aren't covered under this scenario, thats why they are there. In theory they should break earlier in the tests, but not necessarily with all the tests.

Every single situation the public directory, js bundle, hello, nested, need to be tested.

When the source directory changes(which it does as per this test when a config file is copied), everything has to be retested or at least partially.

For example, had we tested the index route in the nested pages describe here(using user workspace mock-app), we would have caught the error for #54 which I just found today because one of the duplicate files which was in the correct path wasn't serialized.

When the source directory changes(which it does as per this test when a config file is copied), everything has to be retested or at least partially.

Right good point.

And I think that's a good approach to take here, as for changing something like the userWorkspace, essentially we would want to run everything from the beginning.

One thing that isn't so clear here though, and maybe this could just be made more clear in a describe or it is the specific configuration used and making sure to assert on those specific values somehow?

I would expect to see specific tests to cover each config API being supported (userWorkspace, devServer), even if it means recreating a new config file or amending dynamically in some way to make it as explicit as possible, since we should make sure that if the user provides supported configuration items

sourceDirectory

devServer

etc

That we call those out specifically in our testing somehow, like though

return value of initConfig

output of file paths after running build

webpack config?

Although some cases may have some overlap, I think it's OK if it means that we can define and test the critical paths and outputs of a given feature / workflow / etc for the project if it ensures our tests can be as faithful to how a user would also setup / run our code. Providing a great and accurate development / testing environment we'll provide us much more grounded and confident in the usefulness and coverage of our tests.

A little duplication for the sake of clarity and setup is a positive tradeoff / investment in my book 👍

I always like to think to myself, the convenience in making build tools is to the user, not the maintainer.

thescientist13

So sorry, for just catching this

Are we using json or js? In the code it's the former, in the README it's the latter.

I think I would prefer .js since if we want to add more build time features (like webpack plugins, or anything dynamic really) it would only be possible in a JS file.

Plus, a JS file can be linted. 👌

thescientist13 · 2019-04-26T01:05:09Z

test/setup.js

-          testApp: path.join(__dirname, 'fixtures', 'mock-app', 'src')
+          testApp: path.join(__dirname, 'fixtures', 'mock-app', 'src'),
+          userCfgPath: path.join(__dirname, 'fixtures', 'greenwood.config.json'),
+          userCfgRootPath: path.join(__dirname, '..', 'greenwood.config.json')


ah, so basically for mocking the user's project root, where their own greenwood.config.json would be located?

hutchgrant · 2019-04-26T01:10:12Z

.js doesn't lint properly with json formatting

You would have to module.exports = {} which is ugly to ask a user to do that.

thescientist13 · 2019-04-26T01:13:48Z

right, you need to export it as an object

module.exports = {
  source: 'src2',
  /* etc */
}

hutchgrant · 2019-04-26T01:15:53Z

but is that really the simplest config file for the user?

module.exports = {
  source: 'src2',
  publicPath: '/',
  devServer: {
    port: 1984,
    host: 'localhost'
  },
  meta: {}
};

instead of

{
  "source": "src2",
  "publicPath": "/",
  "devServer": {
    "port": 1984,
    "host": "localhost"
  },
  "meta": {}
}

thescientist13 · 2019-04-27T16:04:19Z

test/setup.js

        const context = { 
          ...ctx,
          userSrc: path.join(__dirname, '..', 'src'), // static src
          userTemplates: path.join(__dirname, '..', 'src', 'templates'), // static src/templates for testing empty templates dir, redundant in #38
-          testApp: path.join(__dirname, 'fixtures', 'mock-app', 'src')
+          testApp: path.join(__dirname, 'fixtures', 'mock-app', 'src'),
+          userCfgPath: path.join(__dirname, 'fixtures', 'greenwood.config.js'),


would the mock config make sense being located in the mock-app folder?

Then testApp could be used to get to the paths for the config too. I think in general we will probably want to revisit this for #56 since I want to make sure we are very precise with the setup / teardown of our tests. In case based development, we'll probably favor composition over inheritance.

Maybe each test case can provide it's own custom context to TestSetup to make sure this setup function doesn't grow too large, handle too many cases, and add too much coupling to our tests.

I don't know to me that seems like more code than simply adding variables where needed but I can see how that could be beneficial. Do you have an idea how to go about that?

config could be within mock-app sure, but then to use test-app we'd have to modify it from src and either:

a) hardcode a path within the test itself for src and greenwood.config.js or

b) add an additional context variable specifically for src.

We could do this a hundred times and continue to adjust the tests but I'd prefer to do it after these changes have been applied. So that I can move on this week, in addition to refactoring all these tests at once, not just specific for one single PR or another PR. Why not deal with it at once? This too would be a large change as well. This would actually reduce the amount of work, as the process of merging those changes, testing, then reviewing those changes, with the other PRs would take longer than doing it at once.

The point is while I'm waiting for it to be reviewed, whatever I do to the tests, I want to be able to work on something else. Which I can't, if we hold this back because of preferred organization of test variables.

The point is while I'm waiting for it to be reviewed, whatever I do to the tests, I want to be able to work on something else. Which I can't, if we hold this back because of preferred organization of test variables.

I didn't say / imply that though I don't think? I did say..

I think in general we will probably want to revisit this for #56

So anyway, I'm just trying to chat about the code in front of me, while it is in front of me is all. Please don't feel bad if I talk or ask a lot of questions in PRs, I'm just trying to understand the decision making process from your POV so I can best understand that and try and make equally informed decisions for when that time comes.

thescientist13 · 2019-04-27T16:32:53Z

test/cli.spec.js

@@ -160,4 +160,67 @@ describe('building greenwood with error handling for app and page templates', ()
    await fs.remove(CONTEXT.scratchDir);
  });

+});
+
+describe('building greenwood with user provided config file', () => {


When the source directory changes(which it does as per this test when a config file is copied), everything has to be retested or at least partially.

Right good point.

And I think that's a good approach to take here, as for changing something like the userWorkspace, essentially we would want to run everything from the beginning.

One thing that isn't so clear here though, and maybe this could just be made more clear in a describe or it is the specific configuration used and making sure to assert on those specific values somehow?

I would expect to see specific tests to cover each config API being supported (userWorkspace, devServer), even if it means recreating a new config file or amending dynamically in some way to make it as explicit as possible, since we should make sure that if the user provides supported configuration items

sourceDirectory

devServer

etc

That we call those out specifically in our testing somehow, like though

return value of initConfig

output of file paths after running build

webpack config?

Although some cases may have some overlap, I think it's OK if it means that we can define and test the critical paths and outputs of a given feature / workflow / etc for the project if it ensures our tests can be as faithful to how a user would also setup / run our code. Providing a great and accurate development / testing environment we'll provide us much more grounded and confident in the usefulness and coverage of our tests.

A little duplication for the sake of clarity and setup is a positive tradeoff / investment in my book 👍

I always like to think to myself, the convenience in making build tools is to the user, not the maintainer.

…n/greenwood into rfc/user-config-issue-40

thescientist13

I think this is pretty much there. Just a little input validation and then I think this should be good to merge. ✅

packages/cli/lib/config.js

README.md

…fc/user-config-issue-40

thescientist13 · 2019-04-28T17:08:15Z

packages/cli/lib/config.js

@@ -23,7 +23,7 @@ module.exports = readAndMergeConfig = async() => {
    const userCfgFile = require(path.join(process.cwd(), 'greenwood.config.js'));

    // prepend paths with current directory
-    if (userCfgFile.source) {
+    if (userCfgFile.source && !path.isAbsolute(userCfgFile.source)) {


I think it might be good to be a little more robust in our error handling here, since we're not checking that any of these directories exist first, which I think is important given how fundamental this particular option is to bootstrapping the entire tool.

Also, we can provide direct feedback to the user too. ex.

if (userCfgFile.workspace) { const workspace = userCfgFile.workspace; // 1) is it an absolute path that exists? let's just use it // 2) else if it exists in process.cwd(), then use that // 3) else, give up and error out to the user if(path.isAbsolute(workspace) && fs.existsSync(workspace) { config.workspace = workspace; } else if(fs.existsSync(path.join(process.cwd(), workspace))) { path.join(process.cwd(), workspace) } else { // sorry, we couldn't find your workspace :( // common issues to check might be: // - typo in your workspace directory name, or in greenwood.config.js // - if using relative paths, make sure your workspace is in the same cwd as _greenwood.config.js_ // - consider using an absolute path, e.g. path.join(__dirname, 'my', 'custom', 'path') // <__dirname>/my/custom/path/ // reject() } }

README.md

thescientist13 · 2019-04-28T17:32:18Z

Almost there!! 🏁 🏃

thescientist13

Created a www folder with nothing in it, created a greenwood.config.js in the root

module.exports = {
  workspace: 'www',
  publicPath: '/owen',
  devServer: {
    port: 1981,
    host: 'localhost'
  }
};

Looking good!

$ yarn build
yarn run v1.12.3
$ node ./packages/cli/index.js build
-------------------------------------------------------
Welcome to Greenwood ♻️
-------------------------------------------------------
Reading project config
Initializing project workspace contexts
Generating graph of workspace files...
Scaffolding out project files...
Generate pages from templates...
Writing imports for md...
Writing Lit routes...
setup index page and html
Scaffolding complete.
Building project for production.
Building SPA from compilation...
webpack build complete
...................................
Static site generation complete!
...................................
✨  Done in 11.52s.
Owens-MBP-2:greenwood owenbuckley$ yarn devel
Owens-MBP-2:greenwood owenbuckley$ cat public/index.html
<!DOCTYPE html><html lang="en" prefix="og:http://ogp.me/ns#"><head><style>body {transition: opacity ease-in 0.2s; }
body[unresolved] {opacity: 0; display: block; overflow: hidden; position: relative; }
</style>
    <title>My App</title>

    <meta charset="utf-8">
    <meta name="description" content="Greenwood">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <meta name="mobile-web-app-capable" content="yes">
    <meta name="apple-mobile-web-app-capable" content="yes">
    <meta name="apple-mobile-web-app-status-bar-style" content="black">
    <meta name="description" content="A modern and performant static site generator supporting Web Component based development.">
   
   .
   .
   .
    <eve-app></eve-app>

  <script type="text/javascript" src="/owen/index.861cad5fddf22530b046.bundle.js"></script>

$ yarn develop
yarn run v1.12.3
$ yarn clean && node ./packages/cli/index.js develop
$ rimraf ./.greenwood && rimraf ./public
-------------------------------------------------------
Welcome to Greenwood ♻️
-------------------------------------------------------
Reading project config
Initializing project workspace contexts
Generating graph of workspace files...
Scaffolding out project files...
Generate pages from templates...
Writing imports for md...
Writing Lit routes...
setup index page and html
Scaffolding complete.
Starting local development server
.
.
.
ℹ ｢wdm｣: Compiled successfully.
Directory www has been added
Now serving Development Server available at http://localhost:1981

Docs look good too!

🎉

hutchgrant added 3 commits April 22, 2019 21:19

fix: adding config file read and integrate with context/webpack, addi…

76f9943

…ng additional tests

fix: updating webpack.develop.js

5bfd5d7

fix: remove dev output

25a921f

hutchgrant requested a review from thescientist13 April 23, 2019 01:32

hutchgrant added 2 commits April 22, 2019 21:44

docs: updating readme

88c95c1

Signed-off-by: HutchGrant <h.g.utchinson@gmail.com>

fix: removing additional output

654a80d

thescientist13 reviewed Apr 24, 2019

View reviewed changes

hutchgrant mentioned this pull request Apr 24, 2019

Enabling default app/page templates for user workspace #52

Merged

fix: indentation 2 spaces

14e1f25

thescientist13 added the RFC Proposal and changes to workflows, architecture, APIs, etc label Apr 26, 2019

thescientist13 requested changes Apr 26, 2019

View reviewed changes

hutchgrant added 3 commits April 25, 2019 21:20

fix: .json to .js config files

d349358

docs: chaning .json to .js readme

590ced0

Merge branch 'master' into rfc/user-config-issue-40

bb7e22d

hutchgrant requested a review from thescientist13 April 26, 2019 01:53

thescientist13 reviewed Apr 27, 2019

View reviewed changes

hutchgrant added 3 commits April 28, 2019 01:36

Merge branch 'master' into rfc/user-config-issue-40

5d3f8c0

test: moving greenwood config into mock-app

b53fe73

Merge branch 'rfc/user-config-issue-40' of github.com:ProjectEvergree…

3053247

…n/greenwood into rfc/user-config-issue-40

thescientist13 mentioned this pull request Apr 28, 2019

[RFC] Case based / BDD organization strategy for tests #56

Closed

5 tasks

thescientist13 reviewed Apr 28, 2019

View reviewed changes

packages/cli/lib/config.js Outdated Show resolved Hide resolved

thescientist13 reviewed Apr 28, 2019

View reviewed changes

README.md Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

Merge branch 'master' of github.com:ProjectEvergreen/greenwood into r…

3cd95cb

…fc/user-config-issue-40

thescientist13 mentioned this pull request Apr 28, 2019

As a user I would like develop task to open default browser automatically #62

Open

5 tasks

hutchgrant added 3 commits April 28, 2019 12:23

fix: webpack config merge

7528cea

Merge branch 'master' of github.com:ProjectEvergreen/greenwood into r…

cd810fe

…fc/user-config-issue-40

fix: config source paths in context

12133d3

hutchgrant added 2 commits April 28, 2019 12:50

docs: root directory to workspace directory

1891719

docs: add current working directory note

10151c9

thescientist13 requested changes Apr 28, 2019

View reviewed changes

hutchgrant added 8 commits April 28, 2019 14:17

fix: adding config validation, changing source to workspace

d7ead99

docs: updating readme for config workspace var

42fbc3a

test: update test for workspace instead of source

9fce093

fix: adding additional check for if workspace exists

1a0c400

docs: updating readme to note http:// default

69d9ca4

fix: refactor config validation, some leftover merge cleanup

7e195d8

fix: tiny config refactor

0a542ac

fix: check the devServer exists before checking keys

ebb30ad

thescientist13 approved these changes Apr 30, 2019

View reviewed changes

thescientist13 merged commit d39adf5 into master Apr 30, 2019

thescientist13 deleted the rfc/user-config-issue-40 branch April 30, 2019 23:13

thescientist13 mentioned this pull request Apr 30, 2019

passing a custom publicPath breaks serialization #65

Closed

5 tasks

hutchgrant mentioned this pull request May 16, 2019

fixed absolute path config bug and improved workspace related specs #82

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: User Config file integration #53

RFC: User Config file integration #53

hutchgrant commented Apr 23, 2019

thescientist13 left a comment •

edited

Loading

thescientist13 Apr 24, 2019

hutchgrant Apr 24, 2019

thescientist13 Apr 26, 2019

thescientist13 Apr 24, 2019

hutchgrant Apr 24, 2019

thescientist13 Apr 24, 2019 •

edited

Loading

hutchgrant Apr 24, 2019 •

edited

Loading

thescientist13 Apr 26, 2019

thescientist13 Apr 24, 2019

hutchgrant Apr 24, 2019 •

edited

Loading

thescientist13 Apr 27, 2019

thescientist13 left a comment

thescientist13 Apr 26, 2019

hutchgrant commented Apr 26, 2019 •

edited

Loading

thescientist13 commented Apr 26, 2019 •

edited

Loading

hutchgrant commented Apr 26, 2019 •

edited

Loading

thescientist13 Apr 27, 2019

hutchgrant Apr 27, 2019 •

edited

Loading

thescientist13 Apr 28, 2019 •

edited

Loading

thescientist13 Apr 27, 2019

thescientist13 left a comment

thescientist13 Apr 28, 2019 •

edited

Loading

thescientist13 commented Apr 28, 2019 •

edited

Loading

thescientist13 left a comment •

edited

Loading

RFC: User Config file integration #53

RFC: User Config file integration #53

Conversation

hutchgrant commented Apr 23, 2019

Related Issue

Summary of Changes

thescientist13 left a comment • edited Loading

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

thescientist13 Apr 24, 2019 • edited Loading

Choose a reason for hiding this comment

hutchgrant Apr 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hutchgrant Apr 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thescientist13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hutchgrant commented Apr 26, 2019 • edited Loading

thescientist13 commented Apr 26, 2019 • edited Loading

hutchgrant commented Apr 26, 2019 • edited Loading

Choose a reason for hiding this comment

hutchgrant Apr 27, 2019 • edited Loading

Choose a reason for hiding this comment

thescientist13 Apr 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thescientist13 left a comment

Choose a reason for hiding this comment

thescientist13 Apr 28, 2019 • edited Loading

Choose a reason for hiding this comment

thescientist13 commented Apr 28, 2019 • edited Loading

thescientist13 left a comment • edited Loading

Choose a reason for hiding this comment

thescientist13 left a comment •

edited

Loading

thescientist13 Apr 24, 2019 •

edited

Loading

hutchgrant Apr 24, 2019 •

edited

Loading

hutchgrant Apr 24, 2019 •

edited

Loading

hutchgrant commented Apr 26, 2019 •

edited

Loading

thescientist13 commented Apr 26, 2019 •

edited

Loading

hutchgrant commented Apr 26, 2019 •

edited

Loading

hutchgrant Apr 27, 2019 •

edited

Loading

thescientist13 Apr 28, 2019 •

edited

Loading

thescientist13 Apr 28, 2019 •

edited

Loading

thescientist13 commented Apr 28, 2019 •

edited

Loading

thescientist13 left a comment •

edited

Loading