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

Serve iframe from django + resurrect iodide local-only dev environment #2024

Merged
merged 2 commits into from
Jul 18, 2019

Conversation

wlach
Copy link
Contributor

@wlach wlach commented Jul 15, 2019

  • The iframe is now served via django from the special /notebooks/eval-frame/ endpoint. This is architecturally a little cleaner and paves the way towards Iodide should not hardcode server addresses #1943
  • For testing changes to the editor only, you can now use the (renamed)
    "simple-serve" command.

Pull Request checklist

  • Documentation: If this feature has or requires documentation, the relevant docs have been updated.
  • Changelog: This PR updates the changelog with any user-visible changes.
  • Tests: This PR includes thorough tests or an explanation of why it does not

@wlach wlach requested a review from bcolloran July 15, 2019 19:26
@wlach wlach force-pushed the eval-frame-django-view branch 3 times, most recently from 1d086c0 to 54a220a Compare July 15, 2019 19:39
@wlach wlach changed the base branch from eval-frame-django-view to master July 15, 2019 19:43
@wlach wlach force-pushed the eval-frame-django-view branch from 54a220a to febd017 Compare July 15, 2019 19:44
@wlach wlach self-assigned this Jul 15, 2019
@wlach wlach requested review from jezdez and robotblake July 15, 2019 19:46
@wlach
Copy link
Contributor Author

wlach commented Jul 15, 2019

@bcolloran please review js parts (python portions optional)
@jezdez could you review the python changes? (and their interactions with the js parts, if you feel up to it). likely a prereq to reviewing is understand the "iodide content in an iframe model" that we have -- either me or @bcolloran could explain this. maybe we could go over it in the iodide meeting tomorrow if you'll be around
@robotblake this is more an FYI than anything else, but grepping through the PR for "middleware" might highlight areas of concern from a deployment POV

@robotblake
Copy link
Contributor

I don't see anything I'd be concerned about off the top of my head.

Copy link
Contributor

@bcolloran bcolloran left a comment

Choose a reason for hiding this comment

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

All seems to work as desired in the django + npm run start mode.

in simple-serve mode, there seems to be a port mismatch somewhere -- i'm getting:
Failed to execute ‘postMessage’ on ‘DOMWindow’: The target origin provided (‘http://localhost:8000’) does not match the recipient window’s origin (‘http://localhost:8080’).,
and the eval-frame is not loading. for simple-serve, you might still need to tweak IODIDE_EVAL_FRAME_ORIGIN and IODIDE_EDITOR_ORIGIN in webpack or something like that?

@bcolloran
Copy link
Contributor

(oh also: there aren't really an JS changes yet in this -- i expect you have other stuff coming in follow-up)

@wlach
Copy link
Contributor Author

wlach commented Jul 16, 2019

@bcolloran the port issue comes from the fact that currently webpack is picking up some stuff from .env, where we had specified these values with our dockerflow-based environment in mind. Oops. For now I'll address this by just serving from port 8000 again, as we do for the internal server -- we can change this later after I move to the next phase of this work, which is to stop specifying server names inside our webpack-generated js altogether.

Copy link
Contributor

@bcolloran bcolloran left a comment

Choose a reason for hiding this comment

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

sounds good, thanks will

@wlach wlach requested review from rafrombrc and removed request for jezdez July 17, 2019 17:36
Copy link
Collaborator

@rafrombrc rafrombrc left a comment

Choose a reason for hiding this comment

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

Looks pretty straightforward to me, +1.

@wlach wlach force-pushed the eval-frame-django-view branch from 107d5e2 to 5012b8f Compare July 18, 2019 15:12
wlach added 2 commits July 18, 2019 11:13
This will allow us to swap in metadata used by the eval frame on the server
side, without recompiling the webpack assets. It is a necessary prerequisite
for iodide-project#1943 ("Iodide should not hardcode server assets").
For testing changes to the editor only, you can now use the (renamed)
"simple-serve" command.
@wlach wlach force-pushed the eval-frame-django-view branch from 5012b8f to 5d96dea Compare July 18, 2019 15:14
@wlach wlach merged commit 4ea8abe into iodide-project:master Jul 18, 2019
@wlach wlach deleted the eval-frame-django-view branch July 18, 2019 15:40
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.

4 participants