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.

Sign up for GitHub

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

Jump to bottom

Fix: Bring Your Own Express #4220

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix: Bring Your Own Express #4220

wants to merge 2 commits into from

Conversation

peterpme
Copy link

@peterpme peterpme commented Apr 15, 2017 •
edited
Loading

Description of changes

Reintroduce BYO (bring-your-own) express to Keystone by:

  • consuming customApp within initExpressApp
  • passing customApp it into createApp

Testing

  • Please confirm npm run test-all ran successfully*

*Caveat: Cannot find module jade takes place on both master and my own branch. The first couple hundred tests pass, but I get snagged on the error below. I will look into this on my end as well but I wanted to open up the PR regardless

[Group002 App / Ux Test App View] Test Suite:

Running:  AdminUI app should allow navigating to the front page by clicking the Front Page Icon

Error thrown for request: /
Error: Cannot find module 'jade'
    at Function.Module._resolveFilename (module.js:470:15)
    at Function.Module._load (module.js:418:25)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at new View (/Users/peter/Sites/keystone/node_modules/express/lib/view.js:80:30)
    at Function.render (/Users/peter/Sites/keystone/node_modules/express/lib/application.js:570:12)
    at ServerResponse.render (/Users/peter/Sites/keystone/node_modules/express/lib/response.js:966:7)
    at /Users/peter/Sites/keystone/test/e2e/routes/index.js:3:7
    at Layer.handle [as handle_request] (/Users/peter/Sites/keystone/node_modules/express/lib/router/layer.js:95:5)
    at next (/Users/peter/Sites/keystone/node_modules/express/lib/router/route.js:137:13)
    at Route.dispatch (/Users/peter/Sites/keystone/node_modules/express/lib/router/route.js:112:3)
    at Layer.handle [as handle_request] (/Users/peter/Sites/keystone/node_modules/express/lib/router/layer.js:95:5)
    at /Users/peter/Sites/keystone/node_modules/express/lib/router/index.js:281:22
    at Function.process_params (/Users/peter/Sites/keystone/node_modules/express/lib/router/index.js:335:12)
    at next (/Users/peter/Sites/keystone/node_modules/express/lib/router/index.js:275:10)
    at /Users/peter/Sites/keystone/node_modules/grappling-hook/index.js:198:10
GET / 500 3.279 ms
 ✖ Expected element <body > h2> text to equal: "Welcome to the e2e test front page...make sure they all pass :=)" - element was not found  - expected "present" but got: "not present"

pass in app into initExpressApp to bring back byo express
ed7a2bd 
dont assign app until initExpress does its thing

pass in customApp into initExpressApp and start
@peterpme peterpme changed the title Fix Bring Your Own Express Fix: Bring Your Own Express Apr 15, 2017
@peterpme peterpme mentioned this pull request Apr 15, 2017
Open
@peterpme
Copy link
Author

The test failed but didn't have anything to do with what I've been working on so I'm not sure what to do:

 Expected element <.EditForm-container [data-field-name=fieldB][data-field-type=location] [data-field-location-path="fieldB.number"]> to be visible - element was not found  - expected "visible" but got: "not found"
    at Array.forEach (native)
    at Object.Location field should show correctly in the edit form (/home/travis/build/keystonejs/keystone/test/e2e/adminUI/tests/group006Fields/testLocationField.js:131:29)
    at _combinedTickCallback (internal/process/next_tick.js:73:7)
    at process._tickCallback (internal/process/next_tick.js:104:9)

@peterpme
Copy link
Author

bump

@CAYdenberg
Copy link
Contributor

CAYdenberg commented May 29, 2017 •
edited
Loading

Cool. I was having the same problem ... if I just install jade without saving it in package.json (npm i jade) the tests pass. Not sure why it's not in the devDepedencies.

@CAYdenberg
Copy link
Contributor

This fixes the tests locally keystonejs/keystone#4346

@peterpme
Copy link
Author

bump

@jcreamer898
Copy link

Any updates here? Was really hoping to use v4 of keystone, but hoping to use it within some of our existing express stuff. Would love to get custom express working again!

@CAYdenberg
Copy link
Contributor

FYI @jcreamer898 it's still doable just not pretty: see https://github.com/ExchangeJS/exchangejs.com/blob/master/app.js#L78 for an example how I've got it set up.

@jcreamer898
Copy link

lol thanks @CAYdenberg

epinapala
epinapala reviewed Apr 19, 2018
View reviewed changes
package.json
@@ -1,6 +1,6 @@
{
"name": "keystone",
"version": "4.0.0-beta.5",
"name": "@peterpme/keystone",
Copy link

Choose a reason for hiding this comment

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

shouldn't this be just keystone while being merged to keystonejs:master ?

package.json
"name": "keystone",
"version": "4.0.0-beta.5",
"name": "@peterpme/keystone",
"version": "4.0.0-beta.6",
Copy link

Choose a reason for hiding this comment

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

may be change the version to next beta?

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

@epinapala epinapala epinapala left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@peterpme @CAYdenberg @jcreamer898 @epinapala @stennie