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

Use MutationObserver to observe log for work instance_linker.js correctly #68

Merged
merged 3 commits into from
Apr 27, 2018

Conversation

inohiro
Copy link
Contributor

@inohiro inohiro commented Jun 26, 2017

instance_linker.js that makes hyperlink from child job instance to parent job instance had not worked correctly. This PR changes it works fine.

logs are appended to #logs element by jQuery. So we need to observe changes of the element with MutationObserver.

I'm not sure about JavaScript, if someone have better idea to solve it, please give me comments.

before

screen shot 2017-06-26 at 11 56 10

after

screen shot 2017-06-26 at 13 02 45

@inohiro inohiro changed the title Use MutationObserver to oberve log for work instance_linker.js correctly Use MutationObserver to observe log for work instance_linker.js correctly Jun 26, 2017
@inohiro
Copy link
Contributor Author

inohiro commented Jun 26, 2017

oh CI failed... I'll confirm it. Maybe arrow function...

@inohiro inohiro force-pushed the link_to_parent_from_child_process branch from df34ab7 to 78a1cc1 Compare June 26, 2017 20:37
}
var logParent = document.querySelector('#logs tbody');
var observer = new MutationObserver(function(mutations) {
if (mutations.some(function(m) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about separate as a named function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should do that 👍

);
var MutationObserver = window.MutationObserver || window.WebKitMutationObserver || window.MozMutationObserver;
if (!MutationObserver) {
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PhantomJS supports MutationObserver from 2.0. But It seems that TravisCI uses PhantomJS 1.x currently (as default). So if browser does not support MutationObserver, this function (instance linker) should be disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But recent modern browsers support MutationObserver like that https://caniuse.com/#feat=mutationobserver

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your information.
To support PhantomJS 2.0 in #70 .
Could you add a test after merged #70 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you about #70. I'll add a test about this.

$('td.log').each(function() {
var logText = $(this).html();
$(this).html(
logText.replace(/instance#(\d+)\/(\d+)(?:\s+|\.|$)/, function(match, jobId, instanceId) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I added |$

@eisuke
Copy link
Member

eisuke commented Jun 27, 2017

I fix CI failing in #69 🙏

@eisuke
Copy link
Member

eisuke commented Jun 27, 2017

Merged #69. Please rebase this branch.

@eisuke eisuke mentioned this pull request Jun 27, 2017
@inohiro inohiro force-pushed the link_to_parent_from_child_process branch from 6de36ea to 5cb4b5f Compare June 27, 2017 23:04
@riseshia
Copy link
Contributor

It seems ready to go. ping @eisuke ?

@eisuke
Copy link
Member

eisuke commented Apr 25, 2018

👍

@riseshia riseshia merged commit 68fdb7d into cookpad:master Apr 27, 2018
@inohiro inohiro deleted the link_to_parent_from_child_process branch April 27, 2018 05:19
@inohiro
Copy link
Contributor Author

inohiro commented Apr 27, 2018

I forgot about this...

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