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

Admin UI static building #1088

Merged
merged 32 commits into from
May 8, 2019
Merged

Admin UI static building #1088

merged 32 commits into from
May 8, 2019

Conversation

emmatown
Copy link
Member

@emmatown emmatown commented May 3, 2019

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented May 3, 2019

✅ This PR has a changeset ✅
Latest commit: cf8a5bc

Click here to learn what changesets are.

@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 3, 2019 10:11 Inactive
@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 3, 2019 10:13 Inactive
@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 3, 2019 10:43 Inactive
@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 3, 2019 11:07 Inactive
@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 3, 2019 11:17 Inactive
@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 3, 2019 22:31 Inactive
@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 3, 2019 22:40 Inactive
@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 3, 2019 22:55 Inactive
@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 3, 2019 23:07 Inactive
@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 4, 2019 02:44 Inactive
@emmatown
Copy link
Member Author

emmatown commented May 4, 2019

This is mostly ready but I have some questions about the CLI @jesstelford:

  • Why are we using the CLI to execute custom servers? From what I understand, a large part of the reason behind using a custom server is being able to run keystone programmatically and using a CLI to execute it defeats that. I'm especially confused because the CLI was modeled after Next's and Next doesn't execute custom servers, people just run node on the server file.
  • Why is the --out flag a thing over an export in index.js? I really like what Next does where you specify the output folder in the Next config so for Keystone, I would think it would be a distDir export from index.js. It's especially nice because it removes the problem of having to specify the dist directory in two commands and in a custom server file if you have one.

@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 5, 2019 21:44 Inactive
@jesstelford
Copy link
Contributor

jesstelford commented May 7, 2019

Why are we using the CLI to execute custom servers?

No particular reason. It's basically a very thin wrapper over just executing the file with node directly.

Why is the --out flag a thing over an export in index.js?

Note that next allows specifying that directory on the command line too (-o):

next export --help

    Description
      Exports the application for production deployment

    Usage
      $ next export [options] <dir>

    <dir> represents where the compiled dist folder should go.
    If no directory is provided, the 'out' folder will be created in the current directory.

    Options
      -h - list this help
      -o - set the output dir (defaults to 'out')
      -s - do not print any messages to console

I don't mind there being a config option too. Personally, I go back and forth between preferring the CLI option and the the config file option for various utils. I like the atomicity of the CLI option - I can run the CLI with different options and not worry about a config file getting in the way. But on the other hand, when there are lots of options, a config file is much easier to read and edit ¯\_(ツ)_/¯

@emmatown
Copy link
Member Author

emmatown commented May 7, 2019

No particular reason. It's basically a very thin wrapper over just executing the file with node directly.

How would you feel about removing it because I feel like it's confusing because you can't use the CLI to pass in options because you have to pass it into keystone.prepare so it misleads people into thinking the CLI is doing something?

@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 8, 2019 02:23 Inactive
@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 8, 2019 02:34 Inactive
Co-Authored-By: mitchellhamilton <mitchell@hamil.town>
@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 8, 2019 02:35 Inactive
@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 8, 2019 02:43 Inactive
@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 8, 2019 02:45 Inactive
@emmatown emmatown requested a review from jesstelford May 8, 2019 02:46
@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 8, 2019 02:46 Inactive
@jesstelford
Copy link
Contributor

What do think about making one of the cypress test projects do build and start instead of running dev so it essentially does exactly that?

Yeah, great idea!

Copy one of the existing tests to a new test file so it's isolated, and we can build on it in the future 👍

@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 8, 2019 03:33 Inactive
@emmatown
Copy link
Member Author

emmatown commented May 8, 2019

Copy one of the existing tests to a new test file so it's isolated, and we can build on it in the future 👍

Do you mean one of the jest tests? I'd rather not do that right now because those particular tests do so much mocking and the internals will change soon because of the servers stuff so the tests would have to change really soon because of that. After the servers stuff is done, I'd like to change those tests to not do very much mocking at all and test everything nicely.


I've made it so the login integration tests run keystone build and start the server with NODE_ENV=production on CI so we have a smoke test for the build and prod start.

I've also fixed the output directory stuff and updated the docs in the root README so it's accurate with what's currently implemented.

Copy link
Contributor

@jesstelford jesstelford left a comment

Choose a reason for hiding this comment

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

Love it! Just a couple cleanups and it's good to merge 👍

Co-Authored-By: mitchellhamilton <mitchell@hamil.town>
@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 8, 2019 06:18 Inactive
@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 8, 2019 06:21 Inactive
@emmatown emmatown requested a review from jesstelford May 8, 2019 06:21
@jesstelford
Copy link
Contributor

Looks like that test you added is catching a bug already! 🎉

@thinkmill-3rdparty-services thinkmill-3rdparty-services temporarily deployed to keystone-5-pr-1088 May 8, 2019 06:35 Inactive
@jesstelford jesstelford merged commit 6f598e8 into master May 8, 2019
@jesstelford jesstelford deleted the static-build-poc branch May 8, 2019 06:50
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.

None yet

3 participants