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

Blocks need to be able to load assets (javascript, css) #634

Closed
notnownikki opened this issue May 3, 2017 · 7 comments
Closed

Blocks need to be able to load assets (javascript, css) #634

notnownikki opened this issue May 3, 2017 · 7 comments
Labels
[Feature] Block API API that allows to express the block paradigm. [Type] Question Questions about the design or development of the editor.

Comments

@notnownikki
Copy link
Member

I'm looking at implementing a block for embedding a tweet.

It seems pretty simple, but there's a wrinkle - we need to load some assets from Twitter. (in this case, some JS).

If we had multiple tweets embedded on a post, we wouldn't want to load the external assets multiple times... so this issue is to discus how we can have blocks register assets that need to be loaded, and have those assets loaded once as part of the post, rather than on the individual blocks.

My first thought is a registerScript method somewhere could be passed a URL of the JS to load, and it wouldn't load things twice.

I'll start on the block on the basis that each instance of the block will load the JS from Twitter, and amend things as this discussion progresses :)

@BE-Webdesign
Copy link
Contributor

This is an excellent point. It will probably need to be addressed when dealing with any of the Embed blocks.

@aduth
Copy link
Member

aduth commented May 3, 2017

Two ideas for block managing this themselves:

  1. The block references its own top-level global indicating whether an asset has already been added (to avoid duplication) and, if falsey, adds to page
var hasLoadedExternal = false;

registerBlock( 'core/foo', {
	edit: class extends wp.element.Component {
		componentDidMount() {
			if ( hasLoadedExternal ) {
				return;
			}

			const script = document.createElement( 'script' );
			script.src = '/path/to/external/resource.js';
			document.body.appendChild( script );
			hasLoadedExternal = true;
		}
	}
} );
  1. Block renders and controls itself as an iframe, which has the advantage of being easy to dispose when removed or otherwise unmounted
registerBlock( 'core/foo', {
	edit: class extends wp.element.Component {
		componentDidMount() {
			const script = document.createElement( 'script' );
			script.src = '/path/to/external/resource.js';
			this.node.contentDocument.body.appendChild( script );
		},

		render() {
			return <iframe ref={ ( node ) => this.node = node } />;
		}
	}
} );

@BE-Webdesign
Copy link
Contributor

Option two looks good 👍

@notnownikki
Copy link
Member Author

I'm not keen on the iframe option, because it doesn't solve the problem of having to load the external resource multiple times. It would also force the external script to run multiple times, and load all of its assets multiple times, whereas it might have detected it was loaded multiple times in a single document and not run. There are also the performance issues with iframes, which are more likely to come up with blocks like the tweet block as they're more likely to be embedded multiple times on a page (think of someone doing a commentary on the latest twitter outrage, there could be a great number of these on a single post!)

I'd prefer an option that looks like a self contained version of option one, which could be used by a developer like this:

registerBlock( 'core/foo', {
	edit: class extends wp.element.Component {
		render() {
			return (
				<p>Foo!</p>
				<ExternalScript src="//some.external.thing.com/js/script.js" />
			);
		}
	}
} );

The ExternalScript component would work like option one, but be completely self contained and reusable, and really easy for a developer to use. It would render the <script> element if it had not been rendered before, and if we're generating the string version of a post by render()ing each block, we get exactly what we expect.

@jasmussen jasmussen added [Feature] Block API API that allows to express the block paradigm. [Type] Question Questions about the design or development of the editor. labels May 4, 2017
@notnownikki
Copy link
Member Author

notnownikki commented May 4, 2017

Further on implementing the embed tweet block, turns out that twitter requires the script block to appear multiple times, as the markup their oembed generates executes the script as soon as it loads, and including the script only once might not render everything, depending on the client's connection.

Still, this might be worth thinking about if we have blocks that need external assets loaded once.

@youknowriad
Copy link
Contributor

Should we close this since the embeds are sandboxed now?

@notnownikki
Copy link
Member Author

Yeah, I think I was expecting things to work slightly differently when I started this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Type] Question Questions about the design or development of the editor.
Projects
None yet
Development

No branches or pull requests

5 participants