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

FEATURE: Warn when not using a number for lastId #231

Merged
merged 1 commit into from
Jul 30, 2020

Conversation

nikolai-b
Copy link
Contributor

By mistake we set lastId = "0" which was silently changed to -1 which took longer than it should have done to track down.

As an aside:

I tried to add a JS spec but I got

jasmine server started
Auto configuration failed
140219735228032:error:25066067:DSO support routines:DLFCN_LOAD:could not load the shared library:dso_dlfcn.c:185:filename(libssl_conf.so): libssl_conf.so: cannot open shared object file: No such file or directory
140219735228032:error:25070067:DSO support routines:DSO_load:could not load the shared library:dso_lib.c:244:
140219735228032:error:0E07506E:configuration file routines:MODULE_LOAD_DSO:error loading dso:conf_mod.c:285:module=ssl_conf, path=ssl_conf
140219735228032:error:0E076071:configuration file routines:MODULE_RUN:unknown module name:conf_mod.c:222:module=ssl_conf

on my machine and when using docker. I'm guessing this is something to do with bazelbuild/rules_closure#353, maybe it is worth looking at headless broswer alternatives to phantomjs?

@eviltrout eviltrout requested a review from SamSaffron July 1, 2020 13:54
@@ -490,6 +490,9 @@
}

if (typeof lastId !== "number") {
if (lastId !== null && typeof lastId !== "undefined") {
console.log("lastId has type " + typeof lastId + " but a number was expected. Setting lastId = -1.")
Copy link
Member

Choose a reason for hiding this comment

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

I am ok with warning against misuse, but console many not be defined, we need to check for it.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe simpler here and more consistent is just to throw if lastId is provided and is invalid.

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 thought about throw but I was worried that would be a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah much prefer to do a breaking change here, hiding this in the console is risky, people will not catch it in CI and so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, I wasn't sure you'd want a breaking change for this but it is much cleaner 👍

@SamSaffron
Copy link
Member

Cool merging this in! Thanks!

@SamSaffron SamSaffron merged commit e309b6d into discourse:master Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants