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

Simplify customization #454

Merged
merged 32 commits into from
Dec 10, 2018
Merged

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Dec 7, 2018

Resolves #438
Resolves #409
Impact: breaking
Type: feature

Various changes based on audits and feedback about difficulties running, building, and customizing.

Changes

  • Moved .yarnrc to .reaction/yarnrc-docker.template and updated the Dockerfile to copy it in. This matches how it's done in component-library and fixes the fact that the yarn config prevents yarn from working outside of Docker.
  • Other changes to Dockerfile to match the one in component-library
  • Move analytics from /config to /src/custom
  • Move next.config.js into /src.
  • Fix NextJS build config so that all of the manual steps are no longer needed.
  • Run the dev server with normal node rather than babel-node. babel-node was unnecessary and slower.
  • Update to latest @reactioncommerce/components-context
  • Add jest-styled-components for tests. This updated all of the snapshots so that styles are also snapshotted.
  • Move setupTests.js from /src to /tests/
  • Add envalid and update everything that uses process.env so that is uses config that is run through envalid for validation and transformation first.
  • Remove remaining Keycloak code that was never used
  • Remove some properties of the AuthStore that are no longer used
  • Move reactionTheme.js and componentTheme.js to /src/custom
  • Move the components context file from /src/lib/theme/components.js to /src/custom/componentsContext.js
  • Move route definition to a new defineRoutes function in /src/custom/routes.js
  • Clean up the default logger object a bit
  • Remove unused appConfig from MobX UIStore
  • Move pagination functions from helpers to utils and get rid of helpers folder
  • Remove unused props from serverRuntimeConfig and publicRuntimeConfig
  • Fix errors on the _error page
  • Move all auth-related server setup out of server.js to a new serverAuth.js
  • Server now listens on PORT env variable, which defaults to 4000 if not set, rather than being hard-coded to 4000.
  • Remove the request NPM package and use fetch instead in the two places where request was used. This removes a large dependency that was unnecessary since fetch is already available.
  • Fix SSR tag grid page has large grid items #409

Breaking changes

Because some of the build steps and file locations, and the way env variables are handled, have changed, this may break custom scripts or tools. In general the defaults here should be much simpler now.

After pulling this in for local development, remove all volumes that begin with reaction-next-starterkit (docker volume rm reaction-next-starterkit_empty_node_modules reaction-next-starterkit_node_modules reaction-next-starterkit_web-yarn) and rebuild the image the first time you run the project (docker-compose up -d --build). Also verify that your .env file has all of the necessary variables based on .env.example

If you have server code that was relying on the fact that babel-node was transpiling it, you'll need to update it to syntax that is compatible with Node 10.

Testing

  • Verify all major aspects of the app continue to work correctly when running in dev mode (docker-compose up -d --build)
  • Verify that you can build for production (see instructions in readme)
  • Verify all major aspects of the app continue to work correctly when running the production Docker image you built.
  • In particular, test logging in and out.
  • Run without env variables set and verify that you get helpful errors on startup now.

Since babel-node isn't used in production, this
will avoid prod-only errors (plus faster)
Primary benefit here is that the .yarnrc file no
longer causes issues with installing out of
Docker
Updates snapshots so that style changes will also be caught in the diffs
And better organize and document app config
Dual layer would mean that unused core
components would still get bundled.
Better to have a single file and expect
conflicts in it.
We already have isomorphic fetch
@aldeed aldeed self-assigned this Dec 7, 2018
@@ -1,22 +1,19 @@
NODE_ENV=development
INTERNAL_GRAPHQL_URL=http://reaction.api.reaction.localhost:3000/graphql-alpha
CANONICAL_URL=http://localhost:4000
Copy link
Contributor Author

@aldeed aldeed Dec 8, 2018

Choose a reason for hiding this comment

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

Updated CANONICAL_URL and removed the unused KEYCLOAK_* ones. The rest is just alphabetized but unchanged.

@@ -8,7 +8,8 @@ require("isomorphic-fetch");
const url = "http://localhost:4000";

describe("NextJS Loading", () => {
it("SSR Loads with an HTML Body", () => fetch(url)
// Skipping this test because it works locally but not on CI. Created issue to solve this soon.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created issue: #456

@aldeed aldeed changed the title [WIP] Feat aldeed simplify customization Simplify customization Dec 10, 2018
@aldeed
Copy link
Contributor Author

aldeed commented Dec 10, 2018

@samkelleher If you have a few minutes, I'd like your feedback on whether these changes help to resolve some of your pain points. There will be additional PRs coming for better separation of pages/data/routing from page presentation and better isolation of all things Stripe for removal.

As you mentioned elsewhere, it's not possible to fully separate customized code from code that likely won't be customized, but the aim of this PR is to at least be a little smarter about that separation to avoid conflicts and confusion when possible.

@samkelleher
Copy link

I like the improvements @aldeed, did you use my fork for reference as have made many of the same changes.
👍🏻 Consolidation of src outside the src directory (like the analytics)
👍🏻 The .yarnrc file being moved
👍🏻 Removal of unused code like keycloak

That said I evolved the babel-node usage rather than remove it so we can write the same node features (namely ES6 modules) in all files (so webpack, server, and browser scripts).

But overall a great good housekeeping and consolidation PR.

@kieckhafer
Copy link
Member

kieckhafer commented Dec 10, 2018

Note for anyone looking at this: I did need to prune my docker volumes (docker system prune --volumes) in order to get this started correctly.

@aldeed
Copy link
Contributor Author

aldeed commented Dec 10, 2018

@samkelleher Yes, I reviewed and borrowed some of your changes from the fork. Thanks! We decided to veer from your approach on babel-node for server code (1) because nearly all ES6 is already supported other than modules and (2) because there is very little server code in this project anyway.

You may be aware, but you could also enable module support in pure node v10 with --experimental-modules flag, though it requires changing all extensions to .mjs. More details here and here

@aldeed
Copy link
Contributor Author

aldeed commented Dec 10, 2018

I updated the description with better instructions on which specific volumes to remove.

Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

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

Everything worked correctly in dev and production mode.

Started without env variables, and the variables were added and things ran smoothly.

@kieckhafer kieckhafer merged commit ae3a176 into develop Dec 10, 2018
@kieckhafer kieckhafer deleted the feat-aldeed-simplify-customization branch December 10, 2018 23:38
@rosshadden
Copy link
Contributor

rosshadden commented Dec 11, 2018

You may be aware, but you could also enable module support in pure node v10 with --experimental-modules flag, though it requires changing all extensions to .mjs.

This is true in general, but in practice there are things you can do to work around it. Such as including a loader like in their example in the node docs:

NODE_OPTIONS='--experimental-modules --loader ./custom-loader.mjs' node x.js

(I bring this up in case it's something we or others wish to do at some point).

@samkelleher
Copy link

I did actually try the --experimental-modules flag in attempt to avoid the babel-node situation. Discussed it internally and based upon it remaining experimental till at least April (after golive) didn't seem worth the risk given our tight timetable.

In my technical experimentation however, it did not like destructured imports, which are used extensively import { this } from "that". So eventually I gave up after spending and went with babel-node just for time wise.

This was referenced Jan 15, 2019
@spencern spencern mentioned this pull request Jan 25, 2019
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.

Dead code audit & clean up SSR tag grid page has large grid items
4 participants