Technical/issue 11 config refactor #43

thescientist13 · 2019-04-18T01:12:16Z

Related Issue

resolves #11

Summary of Changes

Created an internal context object to specifically map directories, paths, etc to be used throughout the build pipeline
Refactored webpack configuration to export a function that returns an object, which we could maybe use to start implementing things like [RFC] User Extensibility - Lifecycles and Plugins #17
A compilation object with the graph and contexts is created in index.js and passed down to all tasks, as the single source of truth
Updated specs to flow a bit more TDDish, add some placeholders for future tasks

Questions

Wasn't sure where to do the webpackMerge, so thought the best place was to do that in each task, since further overriding of any webpack configuration may be more convenient from that particular point in the pipeline. moved commonConfig + webpackMerge into the respective prod / develop configs themselves.

TODO

~~Still a lot of low hanging "magic string" refactoring I can do swap out hardcoded instances of scratchDir, publicDir, etc~~
Might rename scratchDir to outputDir or buildDir for a bit more context - won't do now

hutchgrant · 2019-04-18T01:18:30Z

packages/cli/config/webpack.config.prod.js

+      new HtmlWebpackPlugin({
+        filename: '404.html',
+        template: '.greenwood/404.html',
+        publicPath: '/' // TOOD reuse from common config


should use the config for the publicPath

hutchgrant · 2019-04-18T01:26:47Z

packages/cli/lib/init.js

+        pageTemplate: 'page-template.js',
+        appTemplate: 'app-template.js'
+        // default: true
+      };


This doesn't follow your suggested layout in #11 this comment specifically

With those checks, it's trivial to eliminate #32

This doesn't follow your suggested layout

Sorry, which part isn't being followed?

hutchgrant · 2019-04-18T01:30:28Z

packages/cli/tasks/build.js

+  const commonConfig = require(path.join(__dirname, '..', './config/webpack.config.common.js'))(context);
+  const prodConfig = require(path.join(__dirname, '..', './config/webpack.config.prod.js'))(context);
+  const webpackConfig = webpackMerge(commonConfig, prodConfig);
+


I thought we were keeping webpack configs stuff within webpack configs?

yeah, like i mentioned in the description, I saw two choices

Wasn't sure where to do the webpackMerge, so thought the best place was to do that in each task, since further overriding of any webpack configuration may be more convenient from that particular point in the pipeline

and went with this one based on my initial hunch / intuition, but now that I'm in a clean up phase, I'll take a another pass at this.

hutchgrant · 2019-04-18T01:33:52Z

packages/cli/lib/init.js


  return new Promise((resolve, reject) => {
    try {
-      const usrPagesDir = path.join(process.cwd(), './src/pages');
-      const usrTemplateDir = path.join(process.cwd(), './src/templates');
+      const context = {


why are we changing this to context instead of config? Will this not also contain configuration items?

per our recent slack chats, my thinking had leaned more towards trying to create a an internal state just to represent the paths / directories, etc.

It's very internal and requires careful maintenance on our part, so I don't see it as configuration, but something that could be influenced through configuration, like whatever we decided to make public as part of #40 (like changing the location of the workspace).

hutchgrant · 2019-04-18T01:41:07Z

packages/cli/lib/init.js

-module.exports = initDirectories = async(config) => {
+const userWorkspace = fs.existsSync(path.join(process.cwd(), 'src'))
+  ? path.join(process.cwd(), 'src')
+  : path.join(greenwoodWorkspace, 'templates/');


path.join(greenwoodWorkspace, 'templates/') is used 3 times

hutchgrant · 2019-04-18T01:44:04Z

packages/cli/config/webpack.config.develop.js

+      historyApiFallback: true,
+      hot: false,
+      inline: true
+    },


These variables devServer: { port, host } should potentially be handled by config.

hutchgrant · 2019-04-18T01:51:55Z

packages/cli/index.js

@@ -50,20 +51,32 @@ if (program.parse.length === 0) {
 }

 const run = async() => {
+  process.env.NODE_ENV = MODE === 'develop' ? 'development' : 'production';
+
+  const compilation = await generateCompilation();


This is generally a good idea, read the configs and set the directories before it does anything with the user commands. However, you're now building before running serve or create or any other commands we add. So regardless of whether you leave this here or add it to both runProdBuild or runDevServer, you'll still need to make sure it only runs for those 2 commands not for other potential commands.

However, you're now building before running serve or create or any other commands we add.

good point and in my mind this raises two issues:

we shouldn't be setting process.env.NODE_ENV like we are, since it's just for a simple internal flag and that can be eliminated in remove need to maintain prod and development versions of template files #38 (I will leave a note in that ticket)

We shouldn't even be exposing / maintaining tasks that don't even do anything or even have issues for. I will remove create and serve from index.js

process.env isn't used in the latest PR(which was included in my other PR) anyway, it's unnecessary as the condition its used in is removed for the dev/prod html templates.

We will be adding those tasks eventually thats why they are there.

I don't think that's a correct place for the compile, while those commands are exactly where they need to eventually be(and were planned for).

hutchgrant · 2019-04-18T01:54:13Z

packages/cli/config/webpack.config.develop.js

+      new ManifestPlugin({
+        fileName: 'manifest.json',
+        publicPath


these should use consolidated config's publicPath

hutchgrant · 2019-04-18T01:57:09Z

packages/cli/config/webpack.config.common.js

-  },
+    output: {
+      // TODO magic string - public
+      path: path.join(process.cwd(), 'public'),


should use config publicDir

hutchgrant · 2019-04-18T01:57:18Z

packages/cli/config/webpack.config.common.js

-  },
+    entry: {
+      // TODO magic string - greenwood, app, app.js
+      index: path.join(process.cwd(), '.greenwood', 'app', 'app.js')


should use config scratchDir

hutchgrant · 2019-04-18T04:22:19Z

Should we not also be consolidating the config within the test as well

thescientist13

Should we not also be consolidating the config within the test as well

Not sure what you mean there, but I can will take a look at the CONFIG object can be cleaned up some more.

hutchgrant · 2019-04-18T11:00:16Z

I know it's early but the previous citation object should seem familiar to the one you just committed for the init.js. The point of the issue was to consolidate the config. So I mean one less config to manage would be better.

thescientist13 · 2019-04-18T11:10:20Z

Ok

refactored the webpack prod / dev configs to pull in common config, so the tasks doesn't need to do it
cleaned up more magic string usages (publicPath, scratchDir, publicDir)
Removed unused / unsupported commands serve and create. )I will make issues for tracking those.)

Thinking this is pretty much ready to go now.

hutchgrant · 2019-04-18T11:11:58Z

we will have to move the compilation back into the runProdServ and runDevServer in index.js later anyway, so my thinking is that should go back.

thescientist13 · 2019-04-18T11:14:41Z

we will have to move the compilation back into the runProdServ and runDevServer in index.js later anyway, so my thinking is that should go back.

sorry, maybe you can include code snippets / examples or attach the comment to a particular line of code in the PR? Having a little trouble following some of the comments / suggestions you are making.

hutchgrant · 2019-04-18T11:23:59Z

packages/cli/config/webpack.config.develop.js

-
-  mode: 'development',
+module.exports = (context) => {
+  const commonConfig = require(path.join(__dirname, '..', './config/webpack.config.common.js'))(context);


I understand you're importing and initializing in one line, would it not be easier to import it at the top and then initialize it when it's called, within the merge? webpackMerge(commonConfig(context), {})

ah, I see what you mean! yeah, I like that. let me give it a shot.

hutchgrant

To add clarity to my previous point, we just discussed the exact line where the issue is with the compilation. The solution isn't to remove the placeholders for additional commands(those will eventually come regardless) the logical solution is to recognize commands will be there(in index.js) and that we can't run a build before all commands.

I also explained how the test should be consolidated with the main context object(not simply renamed context) from init.js

There's one more code preference issue, I don't know whether its best practice or not, it simply sticks out in the webpack.config.develop.js more so(same issue) than the webpack.config.prod.js

hutchgrant · 2019-04-18T11:54:01Z

packages/cli/config/webpack.config.common.js

+    output: {
+      path: context.publicDir,
+      filename: '[name].[hash].bundle.js',
+      publicPath: '/'


This is the wrong place to hardcode this. Say I want site to run from mydomain.com/mysite/ as root path, well then '/' will lead to incorrect assets (such as manifest) etc. When I said this should be from the config, I meant the overall package config. You're calling it(what I was referring to as config) context now, assuming you'll add a separate config object, in addition to the context object, as part of the compilation object. You can see how I did this in my version of this PR

Certainly this should default to '/', but it should be configurable outside of webpack, by a user if they want. That's in addition to devServer var such host and port(Iwho knows, someone could have that port reserved or want to host on a network), etc. The issue was consolidating the config afterall.

All the other webpack configs are now relying on this publicPath(which is hardcoded).

I assume though you want to address this in #40 and leave it. Just be forewarned, when #40 is addressed, there will be things like this that will have to be modified as well.

but it should be configurable outside of webpack,

for sure. please make an issue. configuration should be reviewed / implemented on a per item basis, so it would have to come after #40 .

thescientist13 · 2019-04-18T22:19:40Z

thanks for the comment, and the detail. 😊

I think one point that you are raising is fair, in that the ticket was initially intended to consolidate configuration and compilation but per our chats, I personally took to interpret what we were really trying to solve was this issue of "context" (e.g. an element of our overall compilation state), and not necessarily configuration in the sense that we would want all of this context configurable, since it would be a nightmare for error handling and filesystem checking, and would add to much surface area to this small project too soon..

For that reason, I created #40 to set that up, but realize I never updated #11 to reflect that the work would instead be more around lock stepping and consolidating of things like directories and paths, but that we aren't intending to make it public in any way (at least at this time) and that opening "true" configuration, e.g. a way to influence the state of the system, would be done, starting with the userWorkspace directory as a start (like we want for the website work in this repo), in #40

So I think first and foremost is updating #11.

On to your feedback items:

To add clarity to my previous point, we just discussed the exact line where the issue is with the compilation. The solution isn't to remove the placeholders for additional commands(those will eventually come regardless) the logical solution is to recognize commands will be there(in index.js) and that we can't run a build before all commands.

I acknowledge the need for future features and am open to reviewing each one as they come up, but the place to make that case is an issue, not a placeholder in the code.

Right now, we can run the compilation once and it works for the actual commands we are supporting, which is as much as is needed for this refactoring task. Not trying to add more features, or to that effect, maintaining speculative features that aren't even documented somewhere.

From our slack chats, I was keen on trying to hoist as much of the common stuff to the top, like this context, so that it can flow consistently down through the entire build pipeline.

If a compelling use case comes and challenges this working implementation, then there would be no issue landing that change if it resulted in greater value as a result of delivering the underlying feature.

I also explained how the test should be consolidated with the main context object(not simply renamed context) from init.js

Sorry, still not clear, do you have a code example or specific outcome that you are expecting.

Do you mean creating a single file and having them both read from that? From what i see, the context in the test is just used for creating directories, so I suppose we could create a technical task for lock step the tests to the build.

That would be a good idea, some sort of function for setting up / tearing down a mock user workspace in one function? Could be a useful test util, can you make an issue for this! 🙏 (unless I'm still not getting it 😞 )

There's one more code preference issue, I don't know whether its best practice or not, it simply sticks out in the webpack.config.develop.js more so(same issue) than the webpack.config.prod.js

could you elaborate? What is "it" that sticks out in this case?

Also, I have a few issues of my own I want to make after working on this before I merge, so I know it's not all the things, but I think definitely will drive the consistency, predictability, and readability of the project for the better and allow us to move a lot quicker! 🚗

Sometimes it's just about laying the ground work, as tempting as it is to get lots of good things, but doesn't mean they can't, just one change at a time.

So, if I updated #11 and provide my list of follow ups, I hoping we can get this in and crush all the remaining task 💪

thescientist13 · 2019-04-18T22:59:48Z

Well, it looks like i did update the ticket 2 days ago?
#11 (comment)

Maybe it's just an objection to splitting config from this new context? Otherwise most of the A.C. still applies, even without the creation of #40.

Sorry, not trying to be difficult but I think we will be happier making configuration something a lot narrower so we can observe it feature by feature, and let context be more internal for just greenwood to maintain.

hutchgrant · 2019-04-18T23:08:52Z

could you elaborate? What is "it" that sticks out in this case?

specific review comment

If a compelling use case comes and challenges this working implementation, then there would be no issue landing that change if it resulted in greater value as a result of delivering the underlying feature.

We already know of multiple compelling use cases where additional commands will be added. Regardless of whether the placeholders are there or not, it s a matter of when we add them we will have to reverse adding the compilation there and I suggest doing it now to save the hassle. I disagree with moving it in the first place, is my argument, for these reasons.

Basically you're storing clothes/items in the middle of a room because you say nobody is using it. But we know someone will be renting that room eventually in which case we'd have to move it again. Why don't you just store those clothes/items in a proper place so when a person moves in, they won't have to move it themselves?

Do you mean creating a single file and having them both read from that? From what i see, the context in the test is just used for creating directories, so I suppose we could create a technical task for lock step the tests to the build.

edit:
~~You could literally import compile.js, call the generateCompilation(), and get the context for the test that way.~~

You could import the init.js, call the initDirectories(), and get the context for the test that way

There is only a single additional var for the mock-app.

thescientist13 · 2019-04-18T23:34:05Z

We already know of multiple compelling use cases ...

I'll just repeat:
the place to make that case is in an issue, not a placeholder in the code.

You know you want them, but what I have yet to see if the design / proposal for them.

You're pushing hard for keeping code around that isn't doing anything, has no documentation anywhere, and trying to make long terms decisions around it. All I did was what I thought we agreed upon and was in the description and comments of #11 :

hoist configuration / compilation to the top of the build pipeline
lock step all tasks / configs to it
try and replace as many magic strings as possible

I guess I see a compilation as the heart of this build, and the pretty much all tasks would need access to it. What they do with it is up to them, e.g

build a static site for prod
start a dev server
build a static site for prod and server it

that's 3 use cases for a shared compilation, and we only support 2 of them right now! So I don't understand the push to go against what is clearly common. 🤷‍♂️

And i have no idea when we'll have the time actually create a generator (create) in the near future, and by then i bet our design will have probably changed again, so why stress out so much about it now when we have so much to work on already that is adding immediate and continuing value and will help us build our websites?

I think you may have gotten attached this code, and i would suggest, it's OK. The code is still there in version control, and if we really want to support a feature then my recommendation is... you guessed it. Create an issue and open a PR. :)

hutchgrant · 2019-04-18T23:38:13Z

It's not an issue though, you just moved that, why would we create an issue for something you just moved?

Even if you believe there's no use case right now, what harm was it causing back where it was? I argue by moving it now, it creates more problems in the near future.

Yes it's common but when you consider future commands, you're forcing us to step back and move the compilation right back to where it was before you moved it.

thescientist13 · 2019-04-18T23:46:27Z

yeah, i definitely regret now letting them in there in the first place IN #14 , I definitely was not consistent when I should have been. I regret having not said anything then, so as to avoid this bike shedding of a conversation now.

It's not an issue though, you just moved that, why would we create an issue for something you just moved?

Make issues for the two commands you want to add; serve and create. like every other feature that has made its way into this project.

Even if you believe, there's no use case right now, what harm was it causing back where it was?

because it was a mistake for it be there in the first place, so I will take the 🔥 again for it.

hutchgrant · 2019-04-18T23:49:19Z

Blame is on me for putting it there, granted you reviewed and approved it. But again, assuming additional commands, that's where it has to be.

Given the fact we already have CEA, it would be easy to spin up a createscript, or add it as a separate package. Serve we could simply use the local-web-server.

edit
If you want to put it there for now, that's fine, but I hope we can agree that it can't stay there when other commands are added to index.js and that it will eventually have to be moved back.

thescientist13 · 2019-04-20T15:54:25Z

If you want to put it there for now, that's fine, but I hope we can agree that it can't stay there when other commands are added to index.js and that it will eventually have to be moved back.

Honestly, I was only trying to make the case between solving the problem we have now, which is to get greenwood developing and building with a basic idea of a build pipeline / architecture / domain model, vs the problems we don't even have, or need (yet). The fact we got some serious refactoring value out of this at the same time with this change is a bonus.

And I mean, yeah, I'm sure a lot of other things will change too, no need to make it sound ominous, in fact, I hope more changes get made that will continue to improve the confidence / predictability / readability / predictability over time. In fact, proposed many RFCs / improvement that pretty much guarantee that, however I'm not basing a significant amount of my decision making now on them, since they're just ideas I think that are worth tracking.

However you are free to propose changes too, and within that, you can document how something should work / change, with examples, code, etc. That I am happy to acknowledge and consider as influencing other tasks because it's something tangible to actually work against.

Anyway, hope the distinction is clear between the two now. Looking forward to your proposals for those two features and getting them into this project. ✌️

Going to merge this in a bit so we can move forward, since the conversation now isn't really about the substance of the core changes here anymore. Let's talk more on our call.

thescientist13 · 2019-04-22T19:42:52Z

@hutchgrant
Merged #51 , this is ready for your final review / approval 👍

thescientist13 · 2019-04-22T19:46:02Z

😄

thescientist13 added the chore unit testing, maintenance, etc label Apr 18, 2019

thescientist13 requested a review from hutchgrant April 18, 2019 01:12

hutchgrant reviewed Apr 18, 2019

View reviewed changes

thescientist13 commented Apr 18, 2019

View reviewed changes

thescientist13 added 7 commits April 18, 2019 07:12

refactored config and compilation, build task is working

36ae9a0

spec refactoring

8592403

enable more tests

6367490

enable more tests

4582e94

all tests passing

4853c4e

remove comment

d7310b8

develop task working

a8dca41

thescientist13 added 2 commits April 18, 2019 07:15

refactor webpack common / prod config

e1943b7

refactor webpack common / develop config

57e6b23

thescientist13 force-pushed the technical/issue-11-config-refactor branch from 4affb79 to 57e6b23 Compare April 18, 2019 11:15

hutchgrant reviewed Apr 18, 2019

View reviewed changes

hutchgrant requested changes Apr 18, 2019

View reviewed changes

hutchgrant reviewed Apr 18, 2019

View reviewed changes

hoist common config require to top of file

5ac1395

test: consolidating package directory contexts (#51)

7718f70

thescientist13 mentioned this pull request Apr 22, 2019

Refactoring config, fixing app/page template defaults #42

Closed

thescientist13 assigned hutchgrant Apr 22, 2019

hutchgrant approved these changes Apr 22, 2019

View reviewed changes

thescientist13 merged commit 85f57f5 into master Apr 22, 2019

thescientist13 deleted the technical/issue-11-config-refactor branch April 22, 2019 19:46

Technical/issue 11 config refactor #43

Technical/issue 11 config refactor #43

Conversation

thescientist13 commented Apr 18, 2019 • edited Loading

Related Issue

Summary of Changes

Questions

TODO

hutchgrant Apr 18, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hutchgrant Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

hutchgrant Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

thescientist13 Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

hutchgrant Apr 18, 2019 • 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

hutchgrant commented Apr 18, 2019

thescientist13 left a comment

Choose a reason for hiding this comment

hutchgrant commented Apr 18, 2019 • edited Loading

thescientist13 commented Apr 18, 2019

hutchgrant commented Apr 18, 2019

thescientist13 commented Apr 18, 2019

hutchgrant Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hutchgrant left a comment • edited Loading

Choose a reason for hiding this comment

hutchgrant Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

thescientist13 Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

thescientist13 commented Apr 18, 2019

thescientist13 commented Apr 18, 2019

hutchgrant commented Apr 18, 2019 • edited Loading

thescientist13 commented Apr 18, 2019

hutchgrant commented Apr 18, 2019 • edited Loading

thescientist13 commented Apr 18, 2019 • edited Loading

hutchgrant commented Apr 18, 2019 • edited Loading

thescientist13 commented Apr 20, 2019 • edited Loading

thescientist13 commented Apr 22, 2019

thescientist13 commented Apr 22, 2019

thescientist13 commented Apr 18, 2019 •

edited

Loading

hutchgrant Apr 18, 2019 •

edited

Loading

hutchgrant Apr 18, 2019 •

edited

Loading

hutchgrant Apr 18, 2019 •

edited

Loading

thescientist13 Apr 18, 2019 •

edited

Loading

hutchgrant Apr 18, 2019 •

edited

Loading

hutchgrant commented Apr 18, 2019 •

edited

Loading

hutchgrant Apr 18, 2019 •

edited

Loading

hutchgrant left a comment •

edited

Loading

hutchgrant Apr 18, 2019 •

edited

Loading

thescientist13 Apr 18, 2019 •

edited

Loading

hutchgrant commented Apr 18, 2019 •

edited

Loading

hutchgrant commented Apr 18, 2019 •

edited

Loading

thescientist13 commented Apr 18, 2019 •

edited

Loading

hutchgrant commented Apr 18, 2019 •

edited

Loading

thescientist13 commented Apr 20, 2019 •

edited

Loading