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

Require kernel change #2350

Merged
merged 12 commits into from
Nov 30, 2014
Merged

Require kernel change #2350

merged 12 commits into from
Nov 30, 2014

Conversation

JohnMcLear
Copy link
Member

I forked the require kernel so I could make some changes

Mostly the performance (due to # of http requests) was too poor.

https://github.com/ether/etherpad-require-kernel/blob/master/kernel.js#L456 shows the change I made.

We still need to address the issue of it not loading modules that don't serve their main file as /index.js

It needs a lot of testing, I didn't test IE.

So why this version:

  • Reduce # of http requests
  • All tests pass (backend and frontend)
  • Allow loading of modules that don't load from index.js <-- You can see a bodge fix in for async and unorm -- perma fix required for plugins that provide a components.json that includes a main section.. Example cmponents file:
{
  "name": "async",
  "repo": "caolan/async",
  "description": "Higher-order functions and common patterns for asynchronous code",
  "version": "0.1.23",
  "keywords": [],
  "dependencies": {},
  "development": {},
  "main": "lib/async.js",
  "scripts": [ "lib/async.js" ]
}

In this instance our require would be ("async/lib/async.js");

Actually there are only two modules that exhibit this behavior, async and unorm.. For now I have put a hard hack in place for them... It will suffice until we decide what we're doing w/ require kernel and yasjml

  • Minify plugins -- Probably beyond the scope of this PR though -- Minify JS/CSS files for installed plugins into 1 file #1777
  • Have a discussion about bundling the most common plugin functionality into core. Rich text editing flag in settings.json and include (This is outside of tte scope of this PR too)

@JohnMcLear
Copy link
Member Author

Remote beta.etherpad.org tests (running over 40Mb internet connection)

develop (20 plugins)

 - > cache disabled 86 requests 1.3Mb 16.12s
 - > cache enabled 86 requests 99Kb 7.62s

require-kernel-change branch (20 plugins)

 - > cache disabled 63 requests 1.3Mb 15.72s
 - > cache enabled 63 requests 81.6Kb 5.32s

@JohnMcLear
Copy link
Member Author

  • an update of clean-css seems to break things I need to figure out why

I'm struggling to replicate this locally, perhaps I need to clean up after update.. Ah I think the bug only fires when minify is on.

1.17 to 2 breaks it.

Fixed, they changed the API (\o/)

@JohnMcLear
Copy link
Member Author

This is ready for testing. @marcelklehr @luto

JohnMcLear added a commit that referenced this pull request Nov 30, 2014
@JohnMcLear JohnMcLear merged commit b5a0767 into develop Nov 30, 2014
@marcelklehr
Copy link
Contributor

Sorry for being late on this :) - Tested this with a plugin and it loaded http://localhost:9001/javascripts/lib/ep_push2delete/static/js/delete_button.js/index.js?callback=require.define, which is wrong.

@JohnMcLear
Copy link
Member Author

@marcelklehr Afaik it did that before no?

@marcelklehr
Copy link
Contributor

d7e980c is good (http://localhost:9001/javascripts/lib/ep_push2delete/static/js/delete_button.js?callback=require.define)

@JohnMcLear
Copy link
Member Author

ah okay.

@marcelklehr
Copy link
Contributor

I'd change the require-kernel part (https://github.com/ether/etherpad-require-kernel/blob/master/kernel.js#L456) into

var suffixes = ['', '.js', '/index.js'];
if (path.charAt(path.length - 1) == '/') {
  suffixes = ['index.js']; // If it's definitely a path, go to index.js
}
if(path.indexOf('.') === -1) {
  suffixes = ['.js', '/index.js'] // if it's definitely not a fully qualified file check .js and then index.js
}

So, you still save at least one request.

@muxator muxator deleted the require-kernel-change branch November 30, 2019 23:26
@muxator muxator restored the require-kernel-change branch December 7, 2019 18:05
@rhansen rhansen deleted the require-kernel-change branch December 28, 2020 02:35
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.

None yet

2 participants