Skip to content

Conversation

@tylergraf
Copy link

Main Changes

  • set default options for init (ef/hf)
  • Add template files for hf
  • Add .env and /dist to .gitignore
  • Add s3 uploader to .buildpack in template-ef

@joeycozza joeycozza merged commit 39ded8e into develop Feb 22, 2019
@joeycozza joeycozza deleted the hf branch February 22, 2019 18:07
name: 'additionalFeatures',
message: 'What additional features does your app require',
message: 'What additional features does your app require (these are checkboxes)',
default: ['electric-flow', 'header-footer'],
Copy link

Choose a reason for hiding this comment

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

I'm not sure we should default electric-flow to on

Choose a reason for hiding this comment

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

I do not know if this is true, but I was under the assumption that what Tyler did with the HF templating expects EF to be true/on as well.

Tyler would have to chime in further though.

configureEF(appPath, useYarn, ownPath);
}
if (additionalFeatures.includes('header-footer')) {
configureHF(appPath, useYarn, ownPath);
Copy link

Choose a reason for hiding this comment

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

not specific to hf, but I don't think we should allow yarn as an option

Choose a reason for hiding this comment

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

I'm fine with that I guess, but if a developer doesn't use the --use-npm option, then they'll get react, react-dom, and @fs/react-scripts installed with yarn, and then all of "our" dependencies installed with npm.

I don't know if anyone would ever use create-react-app that way though, they should just use the fr-cli.

Choose a reason for hiding this comment

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

I'll go ahead and make this change to remove the yarn option completely in our frontierInit code

const path = require('path');
const waitForWebpack = require('snow/lib/utils/waitForWebpack.js');

const initiatedDirectory = process.env.INIT_CWD;
Copy link

Choose a reason for hiding this comment

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

who sets INIT_CWD?

page.secondaryNav = [
{"href": appPath("/"), title: 'Stuff', "text": 'Stuff'},
];
page.showHelper = true;
Copy link

Choose a reason for hiding this comment

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

do we fell that every app should have helper and secondaryNav setup by default? what about several other page variables that they could enable?

];
page.showHelper = true;
%>
<h1>hi</h1>
Copy link

Choose a reason for hiding this comment

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

Hi to you to. But I don't think it's useful in the default template

alterPackageJsonFile(appPath, appPackage => {
const packageJson = { ...appPackage };
const additionalScripts = {
"heroku-prebuild": "./heroku-prebuild.sh"
Copy link

Choose a reason for hiding this comment

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

can we put this file in a common location instead of copying into each app?

experiments : [
{
// Author/Owner: Tyler Graf
name: 'coolExperiment',
Copy link

Choose a reason for hiding this comment

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

this should follow our naming convention

// Author/Owner: Tyler Graf
name: 'coolExperiment',
description: 'The coolest experiment',
default: true
Copy link

Choose a reason for hiding this comment

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

I would probably do default: false since that is the more common scenario

/build

# development
/dist
Copy link

Choose a reason for hiding this comment

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

wait, we create a dist for dev and a build for prod?

# misc
.DS_Store
.env
.env.local
Copy link

Choose a reason for hiding this comment

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

do we have some sort of pattern for alternate .env files? Should we just do .env.*? Or does something generate files like this, and if so, why?

@@ -0,0 +1 @@
<link rel="manifest" href="%PUBLIC_URL%/manifest.json" />
Copy link

Choose a reason for hiding this comment

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

how does this work with the existing CRA index.html?

@@ -0,0 +1,27 @@
var urlLookup = {
'cas-public-api.cas.ident.service': process.env.BASE_URL + '/cas-public-api',
Copy link

Choose a reason for hiding this comment

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

these fallbacks use the old pattern and should be updated to the new pattern

module.exports = {
experiments : [
{
// Author/Owner: Tyler Graf
Copy link

Choose a reason for hiding this comment

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

I believe we recommend you put author/owner in the description now

}
],
proxyUser: true,
cacheEncryption: true,
Copy link

Choose a reason for hiding this comment

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

Not sure how I feel about proxyUser on by default. And we should discuss making cacheEncryption defult to true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants