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

Support for jsc #11

Merged
merged 2 commits into from
Aug 31, 2017
Merged

Support for jsc #11

merged 2 commits into from
Aug 31, 2017

Conversation

dmikey
Copy link

@dmikey dmikey commented Aug 17, 2017

Issue. The JSC doesn't have a document object, and it's window object is limited, and doesn't have event listening.

Fix. Instead of filling these in the environment, this change prevents window attachments for now. Something more robust could be introduced to capture more environment items automagically. But for now this allows beaver-logger to be used within the JSC for $logger.info and $logger.track calls.

Also looks like the returned object from sync-browser-mocks attaches the data to a parent object, I make the change below to be req.data so that tests can run appropriately. Completely willing to drop that from PR if there is a better solution.

Thanks!

@bluepnume
Copy link
Collaborator

Very cool! Thanks for making the change.

This is something I see regressing really easily though, so any chance you could add a test to make sure things still work if some of the common window props are undefined?

Also: it's 11pm, for god's sake get off github!! :D

@bluepnume
Copy link
Collaborator

(Also good catch on the sync-browser-mocks issue, I guess I hadn't npm installed in a while. Whoops on the semver miss there...)

@dmikey
Copy link
Author

dmikey commented Aug 17, 2017

I'll get one added! And ping you when ready!

@dmikey dmikey closed this Aug 30, 2017
@dmikey dmikey reopened this Aug 31, 2017
@dmikey
Copy link
Author

dmikey commented Aug 31, 2017

Issue #12 is a dependency when this is merged.

@bluepnume bluepnume merged commit 08c90db into krakenjs:master Aug 31, 2017
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.

2 participants