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

refactor(context): Future-proofed context.html and debug.html for modularity #2019

Closed

Conversation

twolfson
Copy link
Contributor

In #1984, we will update context.html and debug.html to centralize the majority Karma's and their logic into scripts. In the next Karma release, we are planning on adding customContextFile and customDebugFile. Since any existing custom context files would be incompatible with this centralization, it would be a breaking change.

To avoid the breaking change/another major release, we want to future-proof context.html and debug.html. In this PR:

  • Relocated window.opener/window.parent logic for context.html into context.js
  • Relocated window.__karma__ definitions in debug.html to context-debug.html
  • Updated debug.html's JS to be override on window.__karma__ rather than define it
    • This will give us leverage if we want something like window.__karma__.stringify to be defined without needing to compile a new context-debug.js

Screenshot of debug.html working:

selection_012

}
};

// Configure our Karma
%CLIENT_CONFIG%
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing I'm uncertain of is if we should call window.__karma__.setupContext(window) here or not. We would need to overwrite window.__karma__ to be a noop in context-debug.js but I can't think of a case when we would need it.

Copy link
Member

Choose a reason for hiding this comment

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

Crazy idea, couldn't we inject the %CLIENT_CONFIG% into the context.js files, and call .setupContext in there. That way we could add it much easier at a later point in time to context-debug.js if we need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could inject CLIENT_CONFIG inside of the .js files but would need to do it in a linter friendly way. For example:

// In client.js
window.__karma__.config = {PLACEHOLDER: 'FOR_CLIENT_CONFIG'}

// In middleware
data = data.replace('{PLACEHOLDER: \'FOR_CLIENT_CONFIG\'}', configJson);

imo, the current setup is good since it's minimal tax to maintain (e.g. no chance of escaping errors) and there are no major benefits to the other setup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With respect to running setupContext in context.js, we can't do that since we would need to override it in debug.html before it's called. Otherwise, we would have overridden window.alert, window.onbeforeunload, and more in debug.html which I believe we don't want

// serve karma.js
if (requestUrl === '/karma.js') {
// serve karma.js, context.js, and context-debug.js
var jsFiles = ['/karma.js', '/context.js', '/context-debug.js']
Copy link
Member

Choose a reason for hiding this comment

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

Let's drop the / in the filenames and add it dynamically on the matches to avoid typo issues in the future.

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'm confused. Do you mean requestUrl.match(/(karma|context|context-debug)\/.js or to always do replacement (effectively an if true)?

@dignifiedquire
Copy link
Member

Can you add the notice about the breaking change in the commit please. See http://karma-runner.github.io/0.13/dev/git-commit-msg.html for how to do that

@twolfson
Copy link
Contributor Author

Technically, it's not a breaking change since the customContextFile edit hasn't been released yet (i.e. it was landed after 0.13.22). But I will oblige and add the notes there

@twolfson twolfson force-pushed the dev/new.context.html.sqwished branch 3 times, most recently from dc7725d to ad487a7 Compare March 25, 2016 19:07
@twolfson
Copy link
Contributor Author

I took care of the linter errors. I'm still confused about so I left it alone

#2019 (comment)

I snuck in a rename from context-debug.js to debug.js since it felt more consistent with context.html/debug.html

@dignifiedquire
Copy link
Member

Alright retriggered CI to make sure those are green, after that I'm going to merge it. Sorry for the delay

@twolfson
Copy link
Contributor Author

I hit "Update branch" but it looks like those are creating merge commits instead of doing a rebase. I'm going to rebase in a bit.

@twolfson twolfson force-pushed the dev/new.context.html.sqwished branch from 2ec875e to d8e65ab Compare April 14, 2016 23:30
@twolfson
Copy link
Contributor Author

Alright, pushed a rebased flavor

@twolfson
Copy link
Contributor Author

Ugh, I lost the commit body in my rebase x_x

…ularity

BREAKING CHANGE:

Our `context.html` and `debug.html` structures have changed to lean on `context.js` and `debug.js`.
This is in preparation for deeper `context.js` changes in karma-runner#1984.

As a result, all `customContextFile` and `customDebugFile` options much update their format
to match this new format.
@twolfson twolfson force-pushed the dev/new.context.html.sqwished branch from d8e65ab to 43f6a1a Compare April 14, 2016 23:33
@twolfson
Copy link
Contributor Author

Aaaand restored:

refactor(context): Future-proofed context.html and debug.html for modularity

BREAKING CHANGE:

Our `context.html` and `debug.html` structures have changed to lean on `context.js` and `debug.js`.
This is in preparation for deeper `context.js` changes in #1984.

As a result, all `customContextFile` and `customDebugFile` options much update their format
to match this new format.

@dignifiedquire
Copy link
Member

Thanks, going to close this in favor of the other PR, so I don't get any issues with merge things as #2020 contains these commits as well

@twolfson
Copy link
Contributor Author

There should be no issues as long as they are landed in order but works for me =P

@twolfson twolfson deleted the dev/new.context.html.sqwished branch April 15, 2016 03:29
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.

None yet

2 participants