Server Views with React by djbarnwal · Pull Request #753 · iodide-project/iodide

djbarnwal · 2018-08-13T22:11:22Z

This Pull Request implements a MVP model of the Iodide website integrated with a server. The user can now -

Save a Notebook on the server
Update it
Fork it
See all notebooks
See Notebooks associated with a user.

What isn't included -

UI is very hacky
Revisions and Versioning of notebooks needs to be hooked with the UI
Handle edge cases (such as fetching 0 notebooks etc.)
Deciding which APIs should be exposed publicly (like viewing all users is possible using api/v1/users/ route)
Write better Webpack config using plugins like Uglify etc.
Write tests for new APIs
Add Linting
Port notebook server options to iframe-2

Closes #669 and #633

working and streamlined for the time when most of our testing will be done using the django server.

…fig-consolidation WIP: consolidate our webpack config

bcolloran · 2018-08-14T18:59:28Z

@djbarnwal the small amount client-side code looks good. requesting review from @wlach on the server stuff.

@djbarnwal please also merge in the latest master

wlach

On the whole this looks ok. After looking at the patch, I am not crazy about django-webpack-loader. It feels pretty unwieldly, and I would rather we just have one static bundle rather than two seperate web configs with the server. However, this will probably be ok for initial testing.

I don't think we need the user api's (at least not yet). Please see if you can get things working with a filter on the notebook endpoints as I suggest, as it will reduce the surface we need to validate and test.

I haven't tested how this actually works, let me know once you've addressed the main points here and I'll take a closer look.

wlach · 2018-08-14T19:23:47Z

 In dev mode, resource paths are set to be relative to the `dev/` directory. Thus, you export a notebook from a dev notebook, you need to be sure save the exported HTML file in the `dev/` folder for the relative paths to correctly resolve the required js/css/font files.

-### Production mode 
+### Production mode


Please try not to let these kind of unrelated changes into your pull requests, as they make them more difficult to review.

wlach · 2018-08-14T19:28:20Z

+urlpatterns = [
+    url(r'^users/$', UserList.as_view(), name='user-list'),
+    url(r'^users/(?P<pk>[0-9]+)/$', UserDetail.as_view(), name='user-detail'),
+    url(r'^users/username/(?P<username>\w+)$', UserDetailFromUsername.as_view(), name='user-detail-username'),


If we were going to keep these methods, use rest_framework's router framework here (see server/notebooks/api_urls.py for an example of this)

wlach · 2018-08-14T19:30:56Z

+  }
+
+  render() {
+    console.log('state', this.state.user)


Please do a search for all console.log statements in your patch and remove them.

wlach · 2018-08-14T19:40:27Z

@@ -0,0 +1,9 @@
+from django.conf.urls import url


I'm pretty sure we don't need these user api's. Instead we can add a "filter by user" on the existing notebooks endpoint:

e.g. /api/v1/notebooks/?user=djbarnwal

See: http://www.django-rest-framework.org/api-guide/filtering/

You can see some examples of setting up filtering with rest framework in @jaredkerim's experimenter project:

https://github.com/mozilla/experimenter/tree/master/app/experimenter/experiments

@wlach This can be done but this way we won't be able to fetch the meta data of the user (such as first name, avatar etc.) for the profile page.

I think it would be more sensible to add such filtering to already existing users route instead of creating another users/username/ route

wlach · 2018-08-14T19:41:17Z

+from server.base.serializers import UserSerializer
+
+
+class UserList(generics.ListCreateAPIView):


If you're going to keep this, use viewsets (probably ReadOnlyModelViewSet) here: http://www.django-rest-framework.org/api-guide/viewsets/

wlach · 2018-08-14T19:44:45Z

    url(r'^notebooks/', include('server.notebooks.urls')),

-    # various views to help with the authentication pipeline
+    # various views to help with the authentication pipe    line


revert this change

wlach · 2018-08-14T19:45:35Z

    url(r'^logout/$', server.views.logout, name='logout'),

+    # route for creating new notebooks
+    url(r'^notebook', server.views.notebook, name='notebook'),


We should probably make this /server/notebooks/new and serve it under the existing notebook urls, above ^^^

wlach · 2018-08-14T19:47:38Z

+  }
+
+  render() {
+    return (


Let's take the advertising spiel out for now, and just focus on presenting a reasonable user interface when people are using the alpha server.

wlach · 2018-08-14T20:02:07Z

-    url(r'^$', server.views.index, name='index'),
+    # url(r'^$', server.views.index, name='index'),
+    # react bundles to views
+    url(r'^(?!oauth/|notebooks)', server.views.index, name='index'),


I'm not sure what the comment here means, or the rationale behind this change? I'm sure there is one, I just don't understand. :)

wlach · 2018-08-14T20:02:50Z

+ </head>
+
+ <body>
+ <div id="react"></div>


Could we pick a more meaningful identifier than react?

…redirect Add https redirect for production deploys

wlach · 2018-08-20T18:49:16Z

This PR seems to have gotten disconnected from master somehow, there's a lot of commits in here that duplicate things already landed.

In any case, after some discussion and thinking, I'd like to fork this PR and try an alternate approach described here:

https://hackernoon.com/reconciling-djangos-mvc-templates-with-react-components-3aa986cf510a

The key benefits are that we won't need seperate package.json files, or a seperate webpack server running on the host or in production. Everything can be generated in the existing npm build step. This should also make load times considerably faster (since we can just prepopulate what's visible on each page on the server side, instead of having the client make a round trip to the server after loading the assets)

I'll do up some experiments based on the code here and we can see how it plays out.

wlach · 2018-09-05T15:41:37Z

I think we can close this for now, thank you @djbarnwal for taking the first crack at this! Even though we decided to move in a slightly different direction for the implementation in the end, the learnings we got from this PR were extremely valuable.

djbarnwal and others added 15 commits August 5, 2018 19:46

Intial Templates

2dc068c

a bunch of changes to consolidate our webpack config and get things

c39a8e4

working and streamlined for the time when most of our testing will be done using the django server.

tweak build-server

60a4d0e

Basic website model

bffeecf

Homepage and User APIs

2c79a92

Further simplification and consolidation of the webpack config

cbc72c2

Merge pull request iodide-project#752 from iodide-project/webpack-con…

398d8b8

…fig-consolidation WIP: consolidate our webpack config

add build-ghpages to package.json

b1fef8e

Add username api route and user page

158b83d

fix typo at "startsWith"

442cbdf

removing cp step from travis build

2bb4739

Add server options to notebook

e07c366

Clean and Refactor

341b036

Serve static react bundle

2b41885

Update Readme

77e0aca

bcolloran requested a review from wlach August 14, 2018 18:58

wlach suggested changes Aug 14, 2018

View reviewed changes

wlach and others added 12 commits August 15, 2018 13:46

Update various python dependencies (iodide-project#759)

4234395

Redirect to https version of server in non-debug mode

9165e41

Specify fqdn for iodide eval frame pages target

6c9d50c

Merge pull request iodide-project#760 from iodide-project/secure-ssl-…

8648304

…redirect Add https redirect for production deploys

Intial Templates

d1d6ca9

Basic website model

5fb433e

Homepage and User APIs

dadd706

Add username api route and user page

90e2260

Add server options to notebook

9bcd66c

Clean and Refactor

5dfaa9b

Serve static react bundle

d2a8a01

Update Readme

79875f9

djbarnwal added 2 commits August 16, 2018 18:38

Fixed merge conflicts

4556256

Resolve merge conflicts 2

143e418

wlach mentioned this pull request Aug 16, 2018

Logins no longer persist #758

Closed

djbarnwal added 2 commits August 20, 2018 20:10

Fix Server API and website

036c625

fix notebook logs

1b09c37

wlach closed this Sep 5, 2018

		from server.base.serializers import UserSerializer


		class UserList(generics.ListCreateAPIView):

Conversation

djbarnwal commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcolloran commented Aug 14, 2018

Uh oh!

wlach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wlach commented Aug 20, 2018

Uh oh!

wlach commented Sep 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

djbarnwal commented Aug 13, 2018 •

edited

Loading