search

graphile / starter

Opinionated SaaS quick-start with pre-built user account and organization system for full-stack application development in React, Node.js, GraphQL and PostgreSQL. Powered by PostGraphile, TypeScript, Apollo Client, Graphile Worker, Graphile Migrate, GraphQL Code Generator, Ant Design and Next.js
https://graphile-starter.herokuapp.com
Other
1.75k stars 220 forks source link

Modify components to support ionic fork #192

Open justinr1234 opened 4 years ago

benjie commented 4 years ago

As it stands, this PR contains a lot of changes; as discussed on Discord, lets break it up into a number of bite-sized PRs that will be easier to review and iterate so we can merge things a bit faster. Please make sure that each PR comes with an explanation why this change is valuable in general (not specifically limited to Ionic, e.g. you might show how the change is also necessary for Create React App). Here's some PR topics that would make sense from this:

Formatting changes

I notice that cypress.yml had the option re-wrapped; this should be handled automatically by prettier which formats all our yaml for us, but if this is not the case, a change that enforces prettier formatting of the relevant files would be welcome.

sudo apt-get update

This fix should be in place for everyone, please send it as a standalone PR.

withApollo.ts -> .tsx

A simple rename of this file could be included such that the diff when you later edit it will be much clearer. Actually; since this is just a file rename I'll do it now... Done.

cors

This is going to be a controversial addition which will require quite a bit of back-and-forth (but I'm generally in favour of merging, when done right); lets get it as a clean PR on its own.

lerna run setup

Raise this as a separate PR including a small mention in the README. If it passes tests I see no reason not to merge it.

Session changes (allowed origins)

Please raise a PR for just this change with a good explanation of why it's needed, whether it's needed only for development, and any security concerns/caveats there may be with it's introduction.

justinr1234 commented 4 years ago

@benjie updated

benjie commented 4 years ago

I missed the CSRF stuff; we should split that out too.