Skip to content

Commit

Permalink
JS: Remove dead state transition validation code
Browse files Browse the repository at this point in the history
Soooo, because `this.state` was initialised to `null` and
not 'start', this code was always actually dead.
`validateStateTransition` had no branch for `oldState` being
null, so always returned `false`. This meant `this.state`
was *never* set to current state, and so in all callbacks,
`oldState` was always `null`!

I validated this again by console.logging `oldState` and
`newState` inside `validateStateTransition`. Here's the output
for when a full build is performed:

```
null -> waiting
null -> fetching
null -> unknown
null -> building
null -> failed
null -> built
null -> launching
```

And for when launching a previously built image
```
null -> launching
null -> ready
```

I also put one just above where `this.state` is set, and that
code was never called.

I was hoping to remove all this code anyway, as it doesn't
actually validate nor test anything - that's really done by
unit tests and integration tests. I thought this would hide
bugs - but turns out, it was a bug itself, and hid nothing.

This also changes the signature of the callback for
`onChangeState`, dropping `oldState` and `newState`. `oldState`
was always `null`, and in our code, we actually don't use
`newState` *at all*. The one place that was conditional on
`oldState` being null is now unconditional, because `oldState`
was always `null`.

Ref jupyterhub#774
  • Loading branch information
yuvipanda committed Oct 9, 2023
1 parent 79a15e2 commit e2985f7
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 44 deletions.
22 changes: 5 additions & 17 deletions binderhub/static/js/index.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,4 @@
/* If this file gets over 200 lines of code long (not counting docs / comments), start using a framework
State transitions that are valid are:
start -> waiting
start -> built
start -> failed
waiting -> building
waiting -> failed
building -> pushing
building -> failed
pushing -> built
pushing -> failed
*/
import ClipboardJS from 'clipboard';
import 'event-source-polyfill';
Expand Down Expand Up @@ -54,7 +44,7 @@ function build(providerSpec, log, fitAddon, path, pathType) {
const buildEndpointUrl = new URL("build", new URL(BASE_URL, window.location.href));
const image = new BinderRepository(providerSpec, buildEndpointUrl, buildToken);

image.onStateChange('*', function(oldState, newState, data) {
image.onStateChange('*', function(data) {
if (data.message !== undefined) {
log.writeAndStore(data.message);
fitAddon.fit();
Expand Down Expand Up @@ -92,16 +82,14 @@ function build(providerSpec, log, fitAddon, path, pathType) {
image.close();
});

image.onStateChange('built', function(oldState) {
if (oldState === null) {
$('#phase-already-built').removeClass('hidden');
$('#phase-launching').removeClass('hidden');
}
image.onStateChange('built', function() {
$('#phase-already-built').removeClass('hidden');
$('#phase-launching').removeClass('hidden');
$('#phase-launching').removeClass('hidden');
updateFavicon(BASE_URL + "favicon_success.ico");
});

image.onStateChange('ready', function(oldState, newState, data) {
image.onStateChange('ready', function(data) {
image.close();
// If data.url is an absolute URL, it'll be used. Else, it'll be interpreted
// relative to current page's URL.
Expand Down
28 changes: 1 addition & 27 deletions js/packages/binderhub-client/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ export class BinderRepository {
this.buildUrl.searchParams.append("build_token", buildToken);
}
this.callbacks = {};
this.state = null;
}

/**
Expand Down Expand Up @@ -128,39 +127,14 @@ export class BinderRepository {
}
}

/**
* @param {string} oldState Old state the building process was in
* @param {string} newState New state the building process is in
* @returns True if transition from oldState to newState is valid, False otherwise
*/
validateStateTransition(oldState, newState) {
if (oldState === "start") {
return (
newState === "waiting" || newState === "built" || newState === "failed"
);
} else if (oldState === "waiting") {
return newState === "building" || newState === "failed";
} else if (oldState === "building") {
return newState === "pushing" || newState === "failed";
} else if (oldState === "pushing") {
return newState === "built" || newState === "failed";
} else {
return false;
}
}

_changeState(state, data) {
[state, "*"].map(key => {
const callbacks = this.callbacks[key];
if (callbacks) {
for (let i = 0; i < callbacks.length; i++) {
callbacks[i](this.state, state || this.state, data);
callbacks[i](data);
}
}
});

if (state && this.validateStateTransition(this.state, state)) {
this.state = state;
}
}
}

0 comments on commit e2985f7

Please sign in to comment.