hooks are being duplicated with each pagination.paginate() iteration

[PopGoesTheWza]

Describe the bug

• Node.js version: 12.16.3
• Got: 11.1.0
• OS & version: MacOS (latest)

This issue was discovered while investigating #1221

Here you can see two after response hooks. But in the code you provided there's only one. Another bug here?

I need your input on this too :)

Guess what? I have one single afterResponse hook.

Now if I inspect normalizedOptions.hooks.afterResponse in node_modules/got/dist/source/create.js@94, its length double at each iteration (i.e. 2 at page 2, 4 at page 3, 8 at page 4, etc.)

Funny for I was about to add my share to #1220 .

Actual behavior

the hooks.afterResponse array is double with each pagination.paginate() iteration

Expected behavior

the hooks.afterResponse array should not changed during pagination() processing

Code to reproduce

...

Checklist

• I have read the documentation.
• I have tried my code with the latest version of Node.js and Got.

[PopGoesTheWza]

@szmarczak Here is an ugly but functional test case.

// Nicer test case in PR #1231 

[PopGoesTheWza]

Kludgy workaround, add this line first in your pagination.paginate custom function:

      Reflect.deleteProperty(response.request.options, 'hooks');

[szmarczak]

That's actually an invalid use case. The options are merged. You return the request options, which already include hooks. But I'm fixing it rn.

[PopGoesTheWza]

Kludgy workaround, add this line first in your pagination.paginate custom function:

      Reflect.deleteProperty(response.request.options, 'hooks');

Using Got@11.1.3 without the above kludge:

<--- Last few GCs --->

[47929:0x102a79000]    46989 ms: Mark-sweep 2198.0 (2200.2) -> 2197.8 (2200.2) MB, 656.4 / 0.0 ms  (average mu = 0.664, current mu = 0.000) last resort GC in old space requested
[47929:0x102a79000]    47631 ms: Mark-sweep 2197.8 (2200.2) -> 2197.8 (2199.9) MB, 642.6 / 0.0 ms  (average mu = 0.502, current mu = 0.000) last resort GC in old space requested


<--- JS stacktrace --->

==== JS stack trace =========================================

    0: ExitFrame [pc: 0x10097cc39]
Security context: 0x29966fc408d1 <JSObject>
    1: normalizeArguments(aka normalizeArguments) [0x29964f557831] [/Users/guillaumec/rc-dev/rcsf/node_modules/got/dist/source/core/index.js:~288] [pc=0x36c37d63e26](this=0x2996809c04b1 <undefined>,0x2996809c04b1 <undefined>,0x299632356a49 <Object map = 0x29966815f609>,0x2996c06a6ae1 <Object map = 0x299668177f79>)
    2: normalizeArguments(aka normalizeArgument...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory

Writing Node.js report to file: report.20200510.154750.47929.0.001.json
Node.js report completed
 1: 0x1010285f9 node::Abort() (.cold.1) [/Users/guillaumec/.nvm/versions/node/v12.16.3/bin/node]
 2: 0x10008634d node::FatalError(char const*, char const*) [/Users/guillaumec/.nvm/versions/node/v12.16.3/bin/node]
 3: 0x10008648e node::OnFatalError(char const*, char const*) [/Users/guillaumec/.nvm/versions/node/v12.16.3/bin/node]
 4: 0x100187c07 v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [/Users/guillaumec/.nvm/versions/node/v12.16.3/bin/node]
 5: 0x100187ba7 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [/Users/guillaumec/.nvm/versions/node/v12.16.3/bin/node]
 6: 0x100315955 v8::internal::Heap::FatalProcessOutOfMemory(char const*) [/Users/guillaumec/.nvm/versions/node/v12.16.3/bin/node]
 7: 0x10031d9fc v8::internal::Heap::AllocateRawWithRetryOrFail(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [/Users/guillaumec/.nvm/versions/node/v12.16.3/bin/node]
 8: 0x1002e9fed v8::internal::Factory::NewFixedArrayWithFiller(v8::internal::RootIndex, int, v8::internal::Object, v8::internal::AllocationType) [/Users/guillaumec/.nvm/versions/node/v12.16.3/bin/node]
 9: 0x100465fe4 v8::internal::(anonymous namespace)::ElementsAccessorBase<v8::internal::(anonymous namespace)::FastPackedObjectElementsAccessor, v8::internal::(anonymous namespace)::ElementsKindTraits<(v8::internal::ElementsKind)2> >::GrowCapacity(v8::internal::Handle<v8::internal::JSObject>, unsigned int) [/Users/guillaumec/.nvm/versions/node/v12.16.3/bin/node]
10: 0x100617f74 v8::internal::Runtime_GrowArrayElements(int, unsigned long*, v8::internal::Isolate*) [/Users/guillaumec/.nvm/versions/node/v12.16.3/bin/node]
11: 0x10097cc39 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_NoBuiltinExit [/Users/guillaumec/.nvm/versions/node/v12.16.3/bin/node]
12: 0x36c37d63e26
13: 0x1009025e4 Builtins_InterpreterEntryTrampoline [/Users/guillaumec/.nvm/versions/node/v12.16.3/bin/node]
[1]    47929 abort      node packages/crm/dist/accounts.js

[szmarczak]

That's actually an invalid use case. The options are merged. You return the request options, which already include hooks.

Are you sure you're not doing return {...response.request.options}?

[szmarczak]

The tests are passing...

[PopGoesTheWza]

Odd. Tests do pass indeed.But real use case fails without the kludge (I double checked with and without twice.)

But it seems to takes longer to crash though.

I guess I need to debug deeper and check what is happening.

The effective hooks is a merge of the defaults and the current options, right?

[PopGoesTheWza]

Are you sure you're not doing return {...response.request.options}?

Hold on. You're right. And re-reading the pagination.paginate definition, I see that I intepret it not as intended. Returning the options to the next page... It implies though not specifically that it is only the difference from current options.

I am reading it right now?

[szmarczak]

The effective hooks is a merge of the defaults and the current options, right?

yes.

[szmarczak]

I see that I intepret it not as intended.

Cuz I have just updated the readme lol, no worries.

[szmarczak]

It implies though not specifically that it is only the difference from current options.

It explicitly mentions that:

The options are merged automatically with the previous request, therefore the options returned pagination.paginate(...) must reflect changes only.

[PopGoesTheWza]

To summarise:

• a legitimate mistake
• a documentation issue
• new test cases which are of minor interest

[szmarczak]

Actually that's my mistake I didn't document it better... I hope it's clear now.