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

RFC: User Config file integration #53

Merged
merged 26 commits into from
Apr 30, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
76f9943
fix: adding config file read and integrate with context/webpack, addi…
hutchgrant Apr 23, 2019
5bfd5d7
fix: updating webpack.develop.js
hutchgrant Apr 23, 2019
25a921f
fix: remove dev output
hutchgrant Apr 23, 2019
88c95c1
docs: updating readme
hutchgrant Apr 23, 2019
654a80d
fix: removing additional output
hutchgrant Apr 23, 2019
14e1f25
fix: indentation 2 spaces
hutchgrant Apr 24, 2019
d349358
fix: .json to .js config files
hutchgrant Apr 26, 2019
590ced0
docs: chaning .json to .js readme
hutchgrant Apr 26, 2019
bb7e22d
Merge branch 'master' into rfc/user-config-issue-40
hutchgrant Apr 26, 2019
5d3f8c0
Merge branch 'master' into rfc/user-config-issue-40
hutchgrant Apr 28, 2019
b53fe73
test: moving greenwood config into mock-app
hutchgrant Apr 28, 2019
3053247
Merge branch 'rfc/user-config-issue-40' of github.com:ProjectEvergree…
hutchgrant Apr 28, 2019
3cd95cb
Merge branch 'master' of github.com:ProjectEvergreen/greenwood into r…
hutchgrant Apr 28, 2019
7528cea
fix: webpack config merge
hutchgrant Apr 28, 2019
cd810fe
Merge branch 'master' of github.com:ProjectEvergreen/greenwood into r…
hutchgrant Apr 28, 2019
12133d3
fix: config source paths in context
hutchgrant Apr 28, 2019
1891719
docs: root directory to workspace directory
hutchgrant Apr 28, 2019
10151c9
docs: add current working directory note
hutchgrant Apr 28, 2019
d7ead99
fix: adding config validation, changing source to workspace
hutchgrant Apr 28, 2019
42fbc3a
docs: updating readme for config workspace var
hutchgrant Apr 28, 2019
9fce093
test: update test for workspace instead of source
hutchgrant Apr 28, 2019
1a0c400
fix: adding additional check for if workspace exists
hutchgrant Apr 28, 2019
69d9ca4
docs: updating readme to note http:// default
hutchgrant Apr 28, 2019
7e195d8
fix: refactor config validation, some leftover merge cleanup
hutchgrant Apr 28, 2019
0a542ac
fix: tiny config refactor
hutchgrant Apr 28, 2019
ebb30ad
fix: check the devServer exists before checking keys
hutchgrant Apr 28, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ Here are some of the features and capabiliites of Greenwood.

### Configure

Custom greenwood configurations can be added to a `greenwood.config.js` file in your root directory. For example, you may want to change the `src` folder to something else such as `www`.
Custom greenwood configurations can be added to a `greenwood.config.js` file in your root directory. For example, you may want to change the `src` folder to something else such as `www`. By default, you can use a relative path. You can also use an absolute path.

```js
module.exports = {
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/lib/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Member

@thescientist13 thescientist13 Apr 28, 2019

Choose a reason for hiding this comment

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

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()
  }
}

userCfgFile.source = path.join(process.cwd(), userCfgFile.source);
}

Expand Down
36 changes: 16 additions & 20 deletions packages/cli/lib/init.js
Original file line number Diff line number Diff line change
@@ -1,31 +1,27 @@
const fs = require('fs');
const path = require('path');
const greenwoodWorkspace = path.join(__dirname, '..');
const defaultTemplateDir = path.join(greenwoodWorkspace, 'templates/');
const defaultTemplatesDir = path.join(__dirname, '../templates/');
const scratchDir = path.join(process.cwd(), './.greenwood/');
const publicDir = path.join(process.cwd(), './public');

module.exports = initContexts = async({ config }) => {

return new Promise((resolve, reject) => {

try {
const defaultSrc = config.source;

const userWorkspace = fs.existsSync(defaultSrc)
? defaultSrc
: defaultTemplateDir;

const pagesDir = fs.existsSync(path.join(userWorkspace, 'pages'))
? path.join(userWorkspace, 'pages/')
: defaultTemplateDir;

const templatesDir = fs.existsSync(path.join(userWorkspace, 'templates'))
? path.join(userWorkspace, 'templates/')
: defaultTemplateDir;

const context = {
userWorkspace,
pagesDir,
const userWorkspace = path.join(config.source);
const userPagesDir = path.join(userWorkspace, 'pages/');
const userTemplatesDir = path.join(userWorkspace, 'templates/');
const userAppTemplate = path.join(userTemplatesDir, 'app-template.js');
const userPageTemplate = path.join(userTemplatesDir, 'page-template.js');

const userHasWorkspace = fs.existsSync(userWorkspace);
const userHasWorkspacePages = fs.existsSync(userPagesDir);
const userHasWorkspaceTemplates = fs.existsSync(userTemplatesDir);
const userHasWorkspacePageTemplate = fs.existsSync(userPageTemplate);
const userHasWorkspaceAppTemplate = fs.existsSync(userAppTemplate);

let context = {
scratchDir,
publicDir,
pagesDir: userHasWorkspacePages ? userPagesDir : defaultTemplatesDir,
Expand All @@ -40,7 +36,7 @@ module.exports = initContexts = async({ config }) => {
indexPageTemplate: 'index.html',
notFoundPageTemplate: '404.html'
};

if (!fs.existsSync(scratchDir)) {
fs.mkdirSync(scratchDir);
}
Expand Down
80 changes: 80 additions & 0 deletions test/cli.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ describe('building greenwood with user workspace that doesn\'t contain app templ
await fs.remove(CONTEXT.scratchDir);
});
});

describe('building greenwood with user workspace that doesn\'t contain page template', () => {
before(async() => {
setup = new TestSetup();
Expand All @@ -282,6 +283,85 @@ describe('building greenwood with user workspace that doesn\'t contain page temp
blogPageHtmlPath = path.join(CONTEXT.publicDir, 'blog', '20190326', 'index.html');
});

it('should create a public directory', () => {
expect(fs.existsSync(CONTEXT.publicDir)).to.be.true;
});

describe('public directory output', () => {
it('should output a single index.html file (home page)', () => {
expect(fs.existsSync(path.join(CONTEXT.publicDir, './index.html'))).to.be.true;
});

it('should output one JS bundle', async() => {
expect(await glob.promise(path.join(CONTEXT.publicDir, './**/index.*.bundle.js'))).to.have.lengthOf(1);
});

it('should create a default hello page directory', () => {
expect(fs.existsSync(path.join(CONTEXT.publicDir, './hello'))).to.be.true;
});

describe('default generated hello page directory', () => {
const defaultHeading = 'Test App';
const defaultBody = 'This is a test app using a custom user template!';
let dom;

beforeEach(async() => {
dom = await JSDOM.fromFile(path.resolve(CONTEXT.publicDir, 'hello/index.html'));
});

it('should output an index.html file within the default hello page directory', () => {
expect(fs.existsSync(path.join(CONTEXT.publicDir, './hello', './index.html'))).to.be.true;
});

it('should have the expected heading text within the hello example page in the hello directory', async() => {
const heading = dom.window.document.querySelector('h3').textContent;

expect(heading).to.equal(defaultHeading);
});

it('should have the expected paragraph text within the hello example page in the hello directory', async() => {
let paragraph = dom.window.document.querySelector('p').textContent;

expect(paragraph).to.equal(defaultBody);
});
});
});

it('should contain a nested blog page directory', () => {
expect(fs.existsSync(path.join(CONTEXT.publicDir, 'blog', '20190326'))).to.be.true;
});

describe('nested generated blog page directory', () => {
const defaultHeading = 'Blog Page';
const defaultBody = 'This is the blog page built by Greenwood.';
let dom;

beforeEach(async() => {
dom = await JSDOM.fromFile(blogPageHtmlPath);
});

it('should contain a nested blog page with an index html file', () => {
expect(fs.existsSync(blogPageHtmlPath)).to.be.true;
});

it('should have the expected heading text within the blog page in the blog directory', async() => {
const heading = dom.window.document.querySelector('h3').textContent;

expect(heading).to.equal(defaultHeading);
});

it('should have the expected paragraph text within the blog page in the blog directory', async() => {
let paragraph = dom.window.document.querySelector('p').textContent;

expect(paragraph).to.equal(defaultBody);
});
});
after(async() => {
await fs.remove(CONTEXT.userSrc);
await fs.remove(CONTEXT.publicDir);
await fs.remove(CONTEXT.scratchDir);
});

});

describe('building greenwood with user provided config file', () => {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@hutchgrant hutchgrant Apr 24, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Expand Down
3 changes: 2 additions & 1 deletion test/setup.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ module.exports = class Setup {
init() {
return new Promise(async(resolve, reject) => {
try {
const ctx = await initContext({ config: {} });
const defaultSource = path.join(process.cwd(), 'src');
const ctx = await initContext({ config: { source: defaultSource } });
const context = {
...ctx,
userSrc: path.join(__dirname, '..', 'src'), // static src
Expand Down