Improvement/alias queue #1156

dbemiller · 2017-04-25T18:29:38Z

Type of change

[x] Feature

Description of change

This fixes #1124 (Deprecateque in favor of queue)

…example_gpt.html be unix-like, rather than windows-like.

protonate

Looks good, thanks @dbemiller!

snapwich · 2017-04-25T23:17:24Z

this isn't an alias, this is a replacement.

protonate · 2017-04-25T23:19:10Z

ah whoops yep gotta add the alias too, can't break the api, nice catch @snapwich

protonate

pbjs.que needs to continue to work

mkendall07 · 2017-04-25T23:32:57Z

Isn't the alias defined in prebidGlobal.js ?

snapwich · 2017-04-26T00:13:34Z

@mkendall07

window.$$PREBID_GLOBAL$$.que = window.$$PREBID_GLOBAL$$.que || window.$$PREBID_GLOBAL$$.queue;

won't work since all the operations have been renamed to operate on pbjs.queue.

the operations should probably be unchanged to still operate on pbjs.que and then the alias should be something like

window.$$PREBID_GLOBAL$$.que = window.$$PREBID_GLOBAL$$.que || window.$$PREBID_GLOBAL$$.queue || [];

snapwich · 2017-04-26T00:18:27Z

forgot one thing... both pbjs.que.push and pbjs.queue.push need to be monkey-patched.

… tests.

dbemiller · 2017-04-26T17:57:27Z

Thanks guys. I didn't fully understand what the queue was trying to accomplish, before making those changes. I definitely don't intend to break backwards compatibility.

The latest changes split que from queue, log warnings for people who are using the deprecated method, and add a bunch of unit tests.

mkendall07 · 2017-04-26T18:05:18Z

src/prebid.js

-        $$PREBID_GLOBAL$$.que[i].call();
-        $$PREBID_GLOBAL$$.que[i].called = true;
+        if (deprecationWarning) {
+          utils.logWarn('$$PREBID_GLOBAL$$.que.push is deprecated, to be removed in v1.0.0. Use $$PREBID_GLOBAL.queue.push instead.');


This is going to spam the console with deprecation warnings for all consumers of prebid.js. Probably should only display it once.

I must admit... I had kind of intended to spam them :p. The more annoying it is, the faster people will want to update their code ^^.

I must admit there's no substantive benefit, though. I pulled the warning out of the loop.

nanek · 2017-04-27T14:18:55Z

It is possible to just add an alias for queue as suggested in #1124 instead of deprecating que? Then this doesn't have the side effect of breaking all existing code for all customers, documentation and examples. As a user of prebid I prefer the original intent for the naming of being short as stated in #1124, and i'm not opposed to adding an alias if it helps others. However it does seem a little rushed to make que deprecated and a breaking change for all customers without a good benefit.

dbemiller · 2017-04-27T15:20:48Z

So... it sounds like there are two issues here.

Should Prebid as a project use short names, or descriptive ones?

I'm very new to the project, so if I've missed decisions about [1], please point me to them. In isolation, though, I would argue for descriptive names in APIs for several reasons:

They make your code read like English, which makes it easier to understand
They minimize the barrier to entry, since newcomers don't have to learn your acronyms or find your documentation to understand your example code
They're self-describing, so that "occasional" Prebid programmers who come back to their code a month or two later will remember more easily what the function does.

Let me know if you have more reasons than these... but the only benefits I can come up with in favor of shorter names are:

They have a (very tiny) performance benefit
They require fewer keystrokes for people code in an environment which doesn't autocomplete.

I definitely don't want to upset the project philosophy if you've already had this discussion... but in a vacuum, descriptive names have always seemed more beneficial to me. Truth be told, even queue isn't totally descriptive... but it's clearer than que :).

How much community disruption are these changes worth?

I simply don't know the Prebid community well enough yet to judge this, for such a small change.

In general, though, API changes should be expected in a v0 project.

mkendall07 · 2017-04-27T16:31:32Z

@nanek
This wouldn't be a breaking change until 1.0.

snapwich · 2017-04-27T17:32:26Z

As I mentioned in another thread, if we're going to make backwards-compatible breaking changes, I'd recommend cmd. It's a common term. It's still short. It's what google uses so you get the shared education on the terminology.

dbemiller · 2017-05-05T13:26:31Z

We chatted a bit about it here and agreed that it was probably not a big enough API change to bother bugging publishers with deprecation warnings.

So... this should be ready for another look now.

mkendall07 · 2017-05-17T14:51:12Z

@prebid/all-contributors
Please vote on renaming the command queue in prebid.js:

pbjs.cmd
pbjs.queue

Use 👍 or 👎 in the following posts to vote. Thanks

mkendall07 · 2017-05-17T14:51:23Z

pbjs.cmd

mkendall07 · 2017-05-17T14:51:28Z

pbjs.queue

…rovement/alias-queue

dbemiller · 2017-05-18T14:26:50Z

Looks like there's a pretty clear winner ^^. Renamed to cmd now.

…built * 'master' of https://github.com/prebid/Prebid.js: (23 commits) Increment pre version Probed 0.24.0 Release Beachfront adapter - add ad unit size (prebid#1183) Thoughtleadr adapter - fix postMessage (prebid#1207) When prebid server issues a no-bid response, call addBidResponse for every adUnit requested (prebid#1204) Improvement/timeout xhr (prebid#1172) Add native support (prebid#1072) Improvement/alias queue (prebid#1156) Updated documentaion (prebid#1160) Improvement/prebid iframes amp pages (prebid#1119) Fixes prebid#1114 possible xss issue (prebid#1186) Allowed setTargetingForGPTAsync() to target specific ad unit codes. (prebid#1158) updated tag (prebid#1212) Common user-sync (prebid#1144) Rename secureCreatives file and lint (prebid#1203) HIRO Media: Remove batching mechanism and use AJAX instead of JSONP (prebid#1133) Add Support for DigiTrust in Rubicon Adapter (prebid#1201) Upgrade linters to ESLint with stricter code style (prebid#1111) Add dynamic bidfloor parameter to Smart Adserver Adapter (prebid#1194) Bug fix: bids served by secure creatives does not get pushed into _winningBids (prebid#1192) ...

….23.0 to aolgithub-master * commit '136fc37637749a764070c35c03e7e87a5c157947': (33 commits) Added changelog entry. Implemented passing key values feature. Update code to ESlint rules. Prebid 0.24.1 Release tests: drop ie9 browserstack test Audience Network: separate size from format (prebid#1218) Bugfix/target filtering api fix (prebid#1220) Map sponsor request param to endpoint param (prebid#1219) Increment pre version Probed 0.24.0 Release Beachfront adapter - add ad unit size (prebid#1183) Thoughtleadr adapter - fix postMessage (prebid#1207) When prebid server issues a no-bid response, call addBidResponse for every adUnit requested (prebid#1204) Improvement/timeout xhr (prebid#1172) Add native support (prebid#1072) Improvement/alias queue (prebid#1156) Updated documentaion (prebid#1160) Improvement/prebid iframes amp pages (prebid#1119) Fixes prebid#1114 possible xss issue (prebid#1186) Allowed setTargetingForGPTAsync() to target specific ad unit codes. (prebid#1158) ...

dbemiller added 2 commits April 25, 2017 13:17

Replaced que with queue. Made the line terminator characters in pbjs_…

8f59e67

…example_gpt.html be unix-like, rather than windows-like.

Removed some redundant tests.

b6d016f

mkendall07 added the in progress label Apr 25, 2017

dbemiller requested a review from matthewlane April 25, 2017 18:37

matthewlane approved these changes Apr 25, 2017

View reviewed changes

matthewlane added the LGTM label Apr 25, 2017

protonate approved these changes Apr 25, 2017

View reviewed changes

protonate suggested changes Apr 25, 2017

View reviewed changes

protonate added needs update and removed LGTM labels Apr 25, 2017

Separated que from queue, and added a deprecation warning. Wrote unit…

8c593de

… tests.

mkendall07 reviewed Apr 26, 2017

View reviewed changes

dbemiller added 2 commits April 26, 2017 14:44

Limited the number of deprecation warnings to at most 2.

6ed383b

Style fixes.

bdb7c2d

protonate assigned dbemiller Apr 28, 2017

dbemiller added 2 commits May 5, 2017 09:17

Removed the deprecation warnings.

6e5ae12

Merged from master. Resolved merge conflicts.

ec53037

dbemiller removed the needs update label May 5, 2017

dbemiller added the needs review label May 5, 2017

dbemiller added 2 commits May 18, 2017 10:20

Merge branch 'master' of https://github.com/prebid/Prebid.js into imp…

65311e8

…rovement/alias-queue

Renamed queue to cmd, per the github poll.

c90c6ef

Fixed two more occurrences of outdated strings/comments.

0da6aa6

protonate approved these changes May 18, 2017

View reviewed changes

mkendall07 merged commit 29a521e into master May 18, 2017

mkendall07 removed the in progress label May 18, 2017

mkendall07 deleted the improvement/alias-queue branch May 19, 2017 00:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvement/alias queue #1156

Improvement/alias queue #1156

dbemiller commented Apr 25, 2017

protonate left a comment

snapwich commented Apr 25, 2017

protonate commented Apr 25, 2017

protonate left a comment

mkendall07 commented Apr 25, 2017

snapwich commented Apr 26, 2017

snapwich commented Apr 26, 2017

dbemiller commented Apr 26, 2017

mkendall07 Apr 26, 2017

dbemiller Apr 26, 2017

nanek commented Apr 27, 2017

dbemiller commented Apr 27, 2017

mkendall07 commented Apr 27, 2017

snapwich commented Apr 27, 2017

dbemiller commented May 5, 2017

mkendall07 commented May 17, 2017

mkendall07 commented May 17, 2017

mkendall07 commented May 17, 2017

dbemiller commented May 18, 2017

Improvement/alias queue #1156

Improvement/alias queue #1156

Conversation

dbemiller commented Apr 25, 2017

Type of change

Description of change

protonate left a comment

Choose a reason for hiding this comment

snapwich commented Apr 25, 2017

protonate commented Apr 25, 2017

protonate left a comment

Choose a reason for hiding this comment

mkendall07 commented Apr 25, 2017

snapwich commented Apr 26, 2017

snapwich commented Apr 26, 2017

dbemiller commented Apr 26, 2017

mkendall07 Apr 26, 2017

Choose a reason for hiding this comment

dbemiller Apr 26, 2017

Choose a reason for hiding this comment

nanek commented Apr 27, 2017

dbemiller commented Apr 27, 2017

mkendall07 commented Apr 27, 2017

snapwich commented Apr 27, 2017

dbemiller commented May 5, 2017

mkendall07 commented May 17, 2017

mkendall07 commented May 17, 2017

mkendall07 commented May 17, 2017

dbemiller commented May 18, 2017