search

guardian / scribe

DEPRECATED: A rich text editor framework for the web platform
http://guardian.github.io/scribe/
Apache License 2.0
3.51k stars 245 forks source link

Enable server side rendering #438

Closed szyablitsky closed 8 years ago

szyablitsky commented 8 years ago

This patch enables server side rendering, allowing window access at runtime only if window defined.

hmgibson23 commented 8 years ago

Looks good :+1:

rrees commented 8 years ago

Can you check the failing test? Other than that :shipit:

szyablitsky commented 8 years ago

@rrees can you rebuild the CI build? I really think it is timed out. The same test which is failing here, in previous green builds was flagged with red because of execution time (> 100 ms).

rrees commented 8 years ago

@szyablitsky If you check the CircleCI build you'll see there is a issue with an api variable not being present. I can check it out locally but I think it is related to change.

szyablitsky commented 8 years ago

@rrees No, error with api variable does not reproduce every time. If you check all the builds, than you will see that API error is not permanent and the most repeating error is

  commands
    bold
      given content of "<p>1</p>"
        when queryState is executed for the command
          1) should return false

which errors because of timeout

1) commands insertOrderedList given content of "<p>1</p>" when queryState is executed for the command should return false:
     TypeError: Invalid locator
      at Function.webdriver.Locator.checkLocator (/home/ubuntu/scribe/node_modules/scribe-test-harness/node_modules/selenium-webdriver/lib/webdriver/locators.js:247:9)
      at webdriver.WebDriver.findElement (/home/ubuntu/scribe/node_modules/scribe-test-harness/node_modules/selenium-webdriver/lib/webdriver/webdriver.js:880:33)
      at /home/ubuntu/scribe/test/commands.spec.js:34:27
      at promise.ControlFlow.runInFrame_ (eval at <anonymous> (/home/ubuntu/scribe/node_modules/scribe-test-harness/node_modules/selenium-webdriver/lib/goog/base.js:1124:19), <anonymous>:1857:20)
      at goog.defineClass.notify (eval at <anonymous> (/home/ubuntu/scribe/node_modules/scribe-test-harness/node_modules/selenium-webdriver/lib/goog/base.js:1124:19), <anonymous>:2448:25)
      at promise.Promise.notify_ (eval at <anonymous> (/home/ubuntu/scribe/node_modules/scribe-test-harness/node_modules/selenium-webdriver/lib/goog/base.js:1124:19), <anonymous>:564:12)
      at Array.forEach (native)
      at promise.Promise.notifyAll_ (eval at <anonymous> (/home/ubuntu/scribe/node_modules/scribe-test-harness/node_modules/selenium-webdriver/lib/goog/base.js:1124:19), <anonymous>:553:15)
      at goog.async.run.processWorkQueue [as _onTimeout] (/home/ubuntu/scribe/node_modules/scribe-test-harness/node_modules/selenium-webdriver/lib/goog/async/run.js:130:15)
      at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)

It has similar problems in previous green builds

      given content of "<p>1</p>"
        when queryState is executed for the command
          ✓ should return false (110ms)

I'd be happy to dig it, but I'm not so proficient in front-end development and testing. I cloned repo locally and tried to run the tests, but can't manage them to even run. My main field of expertise is Ruby/Rails.

szyablitsky commented 8 years ago

@rrees sorry to disturb, but maybe you have time to check this test?

rrees commented 8 years ago

@szyablitsky Yep, I think I have a working test suite that I need to create a PR for. I think there are issues with the latest Chrome.

If I get that working then I will either merge this or combine the two changes on server-side rendering into one PR.

szyablitsky commented 8 years ago

@rrees thank you! Will be waiting for good news.

rrees commented 8 years ago

@szyablitsky can you check that Scribe 2.1.0 does what you want?

szyablitsky commented 8 years ago

@rrees Sorry, did not notice the window parameter in the function while reviewing. Server side rendering still does not work.