Server side conversions #716

lexoyo · 2018-03-30T18:16:45Z

When Silex saves / opens / publishes a website, it parses the website's DOM to find URLs and make them absolute (when opening) or relative (when saving) or relative to the published version

This PR takes all these operations on the server side, taking advantage of nodejs parsing libraries, and making it possible to parse the DOM without loading any external images or scripts or styles or iframes, and in a cross browser consistent way

This is way cleaner (no more regexp to find the URLs for example), more efficient on the client side, less efficient on the server side, and it fixes long time bugs like #624

Backward compatibility is dropped for this release because I did not manage to port it from client side to server side, but this version is 100% compatible with the one currently on master. So now when you try to open an older website, Silex sends you to https://old.silex.me/ which has the version currently on master. There you can open your website and save it with the version which still has BC on the client side. Then come back to editor.silex.me and you will be able to open it. A BC system is already ready on this PR but it is waiting for the next version to be used.

This reverts commit f502723.

Server side conversions

JbIPS · 2018-03-30T20:57:37Z

dist/server/BackwardCompat.js

+ * used for backward compat
+ * also the static files are taken from //{{host}}/static/Y-Z
+ */
+const LATEST_VERSION = require(Path.resolve(__dirname, '../../package.json')).version.split('.');


FYI, if you start the server with NPM, it sets an env variable for you : process.env.npm_package_version

JbIPS · 2018-03-30T21:05:27Z

dist/server/PublishJob.js

+    // then download all assets
+    // FIXME: should use unifile's batch method to avoid conflicts or the "too many clients" error in FTP
+    //return Promise.all([
+    return sequential([


As previously discussed you should do that. But maybe you want to clean that in another PR

Yes as you told me it is better to test if a file exist and by writing and checking the error

But I would rather do that in an other PR because this current solution is the result of many problems with unifile batch and I am not comfortable changing it now

JbIPS

Mainly you need to address the package.json problem

JbIPS · 2018-03-30T21:06:00Z

dist/server/PublishJob.js

+      assets
+      .filter(file => !!file)
+      .map(file => {
+        if(file.path === 'css/') {


oops old code sorry

JbIPS · 2018-03-30T21:07:25Z

dist/server/PublishRouter.js

+  // publication tasks
+  router.post('/tasks/:task', bodyParser.json(), (req, res, next) => {
+    // init the session (shouldn't it be done by express??
+    req.session.sessionID = req.session.sessionID || uuid.v4();


Yes it should. Did you encounter a case where the session was not set?

I guess I did
I'll remove it and see what happens since we updated express

JbIPS · 2018-03-30T21:11:29Z

dist/server/WebsiteRouter.js

+const express = require('express');
+const fs = require('fs');
+const { JSDOM } = require('jsdom');
+const bodyParser = require('body-parser');


As seen in CE2, you don't need body-parser since express 4.16.0

Concerning JSON: CE adds routes that collide with Silex new routes, e.g. /website/github/get/master/ will be caught by CE as a service called "website/github". So I need to put Silex router before CE router, and so the body needs to be explicit

Concerning text: here I need the body parsed as text, and I can't figure out how to access the body parser from express to use bodyParser.text(

Also in the docs, this is still written to require('body-parser') so I lost too much time with this for now
https://expressjs.com/en/4x/api.html#req.body

finally I removed it from CE2 since it doesn't need a parser (right?)
and I added it here #718

I think the documentation is outdated on this point but the changelog clearly mentioned the new way to parse bodies https://expressjs.com/en/changelog/4x.html

JbIPS · 2018-03-30T21:17:10Z

dist/server/index.js

+const cloudExplorerRouter = new CloudExplorerRouter(rootUrl);
+app.use(new WebsiteRouter(port, rootUrl, cloudExplorerRouter.unifile));
+app.use(new PublishRouter(port, rootUrl, cloudExplorerRouter.unifile));
+app.use(new SslRouter(app));


I know it's arguable, but IMO this file should act as a controller and perform the environment checks (like SSL and such) before using the router.
In that case, the router wouldn't have to worry about the env and the configuration variables should be injected in its constructor.

This will greatly simplify testing.

👍
I'll try to refactor
If you could write this in a separate issue that would be great - not a problem otherwise, I'll try

JbIPS · 2018-03-30T21:19:32Z

package.json

@@ -38,34 +40,33 @@
    "start:electron": "SILEX_ELECTRON=true electron ."
  },
  "dependencies": {
-    "body-parser": "1.13.3",
+    "bluebird": "*",


I don't think that's what you want. Even with a package-lock this file should be human readable (the lock clearly isn't).

Basically, we talked about express version and it's now very difficult to know which one is used here.

i agree but one of the dependencies has a dependency that breaks the production server but works on localhost. I have no idea which and it is really hard to test since I need to deploy every time (and break the server on preprod.silex.me)

ok I decided to give it one more chance, I fixed all versions to the latest available and it seems to work on preprod.silex.me

👍

JbIPS · 2018-03-30T21:22:14Z

src/js/service/cloud-storage.js

+  // });
+	const oReq = new XMLHttpRequest();
+	oReq.onload = function(event) {
+		if(oReq.status === 200) {


What happened to your indentation? (╯°□°）╯︵ ┻━┻

lexoyo · 2018-03-30T22:55:13Z

Thanks, great review, I'll fix all this

lexoyo · 2018-04-03T21:58:44Z

Do you think that everything is fine now?

package.json with fixed versions
removed body-parser
fixed the other things

Only the refactoring and the test of existing files remain

JbIPS · 2018-04-04T18:22:29Z

Yep, it's good to go!

lexoyo · 2018-04-05T08:18:48Z

Let's do it!

lexoyo · 2018-04-05T09:00:06Z

Deployment in progress
Silex is down

lexoyo · 2018-04-05T09:12:21Z

Silex is back up
This is now live, I'll have a look at the server performance statistics soon

lexoyo added 21 commits March 23, 2018 15:02

convert URLs on the server side

a69c945

WIP: Backward compat on the server side

c787edb

removed backward compat and added URL rewrite for CSS

5bda7b9

display errors coming from CE2

00296bc

fix url rewrite for stylesheets, fix file size limit too low

75c86ba

remove apply JSON styles to DOM

bcaf0fe

fix prepare website

f02f256

Revert "remove apply JSON styles to DOM"

442f620

This reverts commit f502723.

ceanup weird notation in JSON saved style

074c572

publish on the server side

7b82796

cleanup dependencies, fixed versions, use gzip compression when possible

f0a85b8

fixed publication on the server side

4e291e0

hide website before css are loaded

8793862

added the whole path in recent files

bc03c34

Merge pull request #711 from lexoyo/server-side-conversions

dd6e16b

Server side conversions

removed file which came back after a rebase

d1c7c3a

fix deploy on gandi

f7fd3b3

fix node version needed to use URL

98a9ad7

fixed node version at compile time

82a51f4

updated npm dependencies

661b8f4

fix default page not open on published site

76ae8f6

lexoyo added the in progress label Mar 30, 2018

added a link to help wiki

513b44c

lexoyo temporarily deployed to silex-editor March 30, 2018 18:31 Inactive

JbIPS reviewed Mar 30, 2018

View reviewed changes

JbIPS requested changes Mar 30, 2018

View reviewed changes

JbIPS mentioned this pull request Mar 30, 2018

update nodejs version, router can be added to a route with a prefix silexlabs/CloudExplorer2#63

Merged

JbIPS assigned lexoyo Mar 30, 2018

changes for @JbIPS code review (thx)

05bbae8

lexoyo temporarily deployed to silex-editor March 31, 2018 13:22 Inactive

fixed versions for npm dependencies

263e08b

lexoyo temporarily deployed to silex-editor April 1, 2018 14:44 Inactive

JbIPS approved these changes Apr 3, 2018

View reviewed changes

lexoyo merged commit 9cfc2a6 into master Apr 5, 2018

lexoyo removed the in progress label Apr 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Server side conversions #716

Server side conversions #716

lexoyo commented Mar 30, 2018 •

edited

Loading

JbIPS Mar 30, 2018

JbIPS Mar 30, 2018

lexoyo Mar 31, 2018

JbIPS left a comment

JbIPS Mar 30, 2018

lexoyo Mar 31, 2018

JbIPS Mar 30, 2018

lexoyo Mar 31, 2018

JbIPS Mar 30, 2018

lexoyo Mar 31, 2018

lexoyo Apr 1, 2018

JbIPS Apr 3, 2018

JbIPS Mar 30, 2018

lexoyo Apr 1, 2018

JbIPS Mar 30, 2018

lexoyo Mar 31, 2018

lexoyo Apr 1, 2018

JbIPS Mar 30, 2018

lexoyo Mar 31, 2018

lexoyo commented Mar 30, 2018

lexoyo commented Apr 3, 2018

JbIPS commented Apr 4, 2018

lexoyo commented Apr 5, 2018

lexoyo commented Apr 5, 2018

lexoyo commented Apr 5, 2018

Server side conversions #716

Server side conversions #716

Conversation

lexoyo commented Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JbIPS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lexoyo commented Mar 30, 2018

lexoyo commented Apr 3, 2018

JbIPS commented Apr 4, 2018

lexoyo commented Apr 5, 2018

lexoyo commented Apr 5, 2018

lexoyo commented Apr 5, 2018

lexoyo commented Mar 30, 2018 •

edited

Loading