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

iFrame loading #8

Closed
mattheworiordan opened this issue Feb 24, 2015 · 18 comments
Closed

iFrame loading #8

mattheworiordan opened this issue Feb 24, 2015 · 18 comments
Assignees
Labels
bug Something isn't working. It's clear that this does need to be fixed.

Comments

@mattheworiordan
Copy link
Member

From what I can tell, the iFrame.html and Javascript associated with it is loaded from the REST / Realtime end point such as https://rest.ably.io/static/iframe.html, see https://github.com/ably/ably-js/blob/master/browser/lib/transport/iframetransport.js#L66-L73.

I believe this approach has the following issues:

  • Versioning is not respected. If you upgrade a client library and change the iFrame code, older versions of the ably-js browser library will now use the newer HTML & Javascript which is not guaranteed to be backwards compatible.
  • We are not using the CDN.

We should perhaps use Grunt to hard code in a version number based on the git tag and link directly to the cdn.

Note all files needed are on the CDN, see http://cdn.ably.io/lib/iframe.js, http://cdn.ably.io/lib/iframe.min.js and http://cdn.ably.io/lib/iframe.html

@mattheworiordan mattheworiordan added the bug Something isn't working. It's clear that this does need to be fixed. label Feb 24, 2015
@paddybyers
Copy link
Member

It needs to be the same origin as the comet endpoint that it's talking to; that's the point of having it.

I agree about the versioning point.

@mattheworiordan
Copy link
Member Author

Ok, well we can modify the grunt build script later to reference a versioned iFrame. The only issue we then have is how does the Realtime system support the versions? It could proxy S3 to do this as our javascript deployer adds version numbers e.g. http://cdn.ably.io/lib/iframe-0.7.1.js

@paddybyers
Copy link
Member

The only issue we then have is how does the Realtime system support the versions? It could proxy S3 to do this as our javascript deployer adds version numbers

The cdn service in the realtime system already proxies any request in /lib or /static to a configured server (currently http://cdn-ably.s3-website-us-east-1.amazonaws.com/).

We need to decide how the ably-js library decides which version of iframe.html to load - either extracts a version string from the src of the ably-js script element, or is built with a reference to a specific version string.

Then we need to make the grunt task that builds/minifies iframe.html build required versions of that and, if necessary, build versioned variants of ably-js with the version string included somewhere.

@mattheworiordan
Copy link
Member Author

@paddybyers the versioning of the iframe.html file is automatically handled by our javascript deployer, it works in the exactly the same way as it does for the JS files by looking at the tagged version, so Grunt does not need to worry about that.

However, in terms of the reference to the iFrame versioned file, that is indeed a bit trickier. Detecting the version number worries me a little as that means the URL to the Javascript file becomes meaningful, and that just doesn't feel right. However, getting Grunt to simply detect the tag and embed could be easy enough, however we'll need to perhaps then change our javascript deployment script to run the build before it deploys to ensure that the reference is indeed up to date as it's quite feasible you would build, commit, tag, and then deploy. WDYT?

@paddybyers
Copy link
Member

I agree that the deploy script, or a post-checkout hook, should perform the build instead of relying on prebuilt versions.

The URL for iframe.html is meaningless to a developer because they will never reference it themselves. What about just inserting the commit id into the URL instead of a tag?

@mattheworiordan
Copy link
Member Author

Sure, commit ID should work, however if you build your script before you commit it will refer to the previous version so we'd need some discipline to build separately and commit. We could do that, and I could simply raise a warning if you're deploying ably-js with a reference to a SHA in a previous commit, WDYT?

@paddybyers
Copy link
Member

however if you build your script before you commit

I was assuming that we would have a deploy remote which would do the build before deploying.

@mattheworiordan
Copy link
Member Author

@paddybyers could do, but a whole lot more work, so would prefer to do that down the line...

@paddybyers
Copy link
Member

Actually why don't we just have a build step that inserts the version string into the iframe url based on reading the package.json ?

@mattheworiordan
Copy link
Member Author

We could do that, but I didn't really like the idea that our deployment scripts deploys a file that is not the same as the static file compiled in our public Github repo.

@paddybyers
Copy link
Member

We could do that, but I didn't really like the idea that our deployment scripts deploys a file that is not the same as the static file compiled in our public Github repo.

But that's the point - as part of building the static files it includes the version in the URL from the package.json. Then that build is repeatable and has no dependency on git, tags or revision.

@mattheworiordan
Copy link
Member Author

Ah, sorry, I misunderstood, sure, that sounds perfect. Can you make that change then?

@paddybyers
Copy link
Member

I've implemented this, but it turns out to be a bit more complex than I thought, because it is not just a matter of referencing a versioned iframe.html; also that iframe.html needs to reference a versioned iframe.js, and there is also the question of how to deal with the minified variants.

What I have now is:

  • when building, grunt reads the current package.json version, say 0.7.3;
  • versioned copies of iframe[.min].js are created (these are simply copies): iframe-0.7.3.js, iframe.min-0.7.3.js;
  • versioned copies of iframe.html are created (these contain an inline explicit reference to the corresponding .js file): iframe-0.7.3.html, iframe.min-0.7.3.html;
  • the Iframe transport in ably.js loads iframe-0.7.3.html;
    the Iframe transport in ably.min.js loads iframe.min-0.7.3.html.

Now, we have to decide which of these various files gets committed and which you deploy, and whether or not to rename the files when you deploy them, or just deployed the already-versioned files generated by grunt.

@paddybyers
Copy link
Member

This is updated based on our discussion.

What I have now is:

  • all versioned iframe*.html files are removed;
  • grunt reads the current package.json version, say 0.7.3;
  • versioned copies of iframe[.min].html are created: iframe-0.7.3.html, iframe.min-0.7.3.html, now including the javascript inline;
  • the Iframe transport in ably.js loads iframe-0.7.3.html;
  • the Iframe transport in ably.min.js loads iframe.min-0.7.3.html.

Can you update the deploy script based on this?

@mattheworiordan
Copy link
Member Author

@paddybyers I have deployed version 0.7.3 of the lib and updated the ably-env javascript deploy command, see https://github.com/ably/infrastructure/commit/0fe20b212ba1359aba73297132f5f9d8fa3ed1a9

@mattheworiordan
Copy link
Member Author

Can you test it and close this once done?

@paddybyers
Copy link
Member

It doesn't look like it's deploying iframe-xxx.html, but is deploying iframe.min-xxx.html.

@paddybyers
Copy link
Member

I've raised infrastructure issues for a couple of problems, but this is all working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. It's clear that this does need to be fixed.
Development

No branches or pull requests

2 participants