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

[WIP] Moving viewer.js into own service, still buggy #1122

Conversation

AaronCritchley
Copy link
Contributor

Moving viewer.js into it's own service in accordance with Issue #1055.

As of time of writing, I'm running into the followiung bug when running gulp test:

Chrome 45.0.2454 (Linux 0.0.0) ERROR
  Uncaught Error: Factory not given and service missing viewer
  at /tmp/47145ce937f497c2d26f372086a6259b.browserify:19159 <- /home/aaron/amphtml/src/asserts.js:62:4

@AaronCritchley AaronCritchley changed the title Moving viewer.js into own service, still buggy [WIP] Moving viewer.js into own service, still buggy Dec 10, 2015
@AaronCritchley
Copy link
Contributor Author

Pinging @cramforce and @erwinmombay.

@cramforce
Copy link
Member

The reason is that history depends on viewer. The easy fix is to swap installation, the better fix is to install the viewer service from the history service (on top of the change to amp.js).

@erwinmombay
Copy link
Member

@AaronCritchley can you try adding:

import {installViewerService} from '../service/viewer-impl';

installViewerService(window);

on src/service/history-impl.js, should get rid of the error

@AaronCritchley
Copy link
Contributor Author

Sadly no luck.

Chrome 45.0.2454 (Linux 0.0.0) ERROR
  Uncaught Error: Factory not given and service missing viewer
  at /tmp/682779060a57382794543b59095dbda0.browserify:19159 <- /home/aaron/amphtml/src/asserts.js:62:4
[21:27:16] 'test' errored after 22 s
[21:27:16] Error: 1
    at formatError (/usr/local/lib/node_modules/gulp/bin/gulp.js:169:10)
    at Gulp.<anonymous> (/usr/local/lib/node_modules/gulp/bin/gulp.js:195:15)
    at emitOne (events.js:77:13)
    at Gulp.emit (events.js:169:7)
    at Gulp.Orchestrator._emitTaskDone (/home/aaron/amphtml/node_modules/gulp/node_modules/orchestrator/index.js:264:8)
    at /home/aaron/amphtml/node_modules/gulp/node_modules/orchestrator/index.js:275:23
    at finish (/home/aaron/amphtml/node_modules/gulp/node_modules/orchestrator/lib/runTask.js:21:8)
    at cb (/home/aaron/amphtml/node_modules/gulp/node_modules/orchestrator/lib/runTask.js:29:3)
    at removeAllListeners (/home/aaron/amphtml/node_modules/karma/lib/server.js:332:7)
    at Server.<anonymous> (/home/aaron/amphtml/node_modules/karma/lib/server.js:343:9)
    at Server.g (events.js:260:16)
    at emitNone (events.js:72:20)
    at Server.emit (events.js:166:7)
    at emitCloseNT (net.js:1521:8)
    at doNTCallback1 (node.js:428:9)
    at process._tickCallback (node.js:350:17)

@cramforce
Copy link
Member

Could you try to use Chrome DevTools or another way to get a stack trace for when or from where viewerFor is called?

@erwinmombay
Copy link
Member

screen shot 2015-12-16 at 2 05 57 pm

still debugging this but heres the error.stack output.

@erwinmombay
Copy link
Member

@AaronCritchley so in viewport.js it looks like we also need to do:

import {installViewerService} from './service/viewer-impl';

installViewerService(window);

/cc @cramforce

@erwinmombay
Copy link
Member

oh and dont forget to add:

import {getElementService} from './service';

in history.js

@AaronCritchley
Copy link
Contributor Author

Changes implemented, I've merged master but feeling somewhat unsure if I've screwed something up with my implementation.

I will squash all of the commits when the PR is in good shape, thanks for being so patient with the whole thing!

@erwinmombay
Copy link
Member

@AaronCritchley yep, just keep the commits for now, they'll be useful for me for debugging until we get everything working. thanks!

@AaronCritchley
Copy link
Contributor Author

Closing as @cramforce has done this work (thanks!) in #1236.

@AaronCritchley AaronCritchley deleted the feature-viewer-to-service branch December 24, 2015 00:09
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.

3 participants