pixeldesu / moduleRaid

:gift: Taking apart webpackJsonp
MIT License
155 stars 45 forks source link

findFunction returns array of undefined #2

Closed pedroslopez closed 4 years ago

pedroslopez commented 5 years ago

Hello there! Thanks for the great work with this package.

I'm running into an issue using findFunction. To my understanding, when a function is passed as the query parameter, it should execute my function with the current module and if I return true from my function, it should add the module that matches to the resulting array. However, line 137 of moduleraid.js is as follows:

results.push(moduleRaid.mObj[index]);

To me, this doesn't really make sense since you're getting property index of the modules object, which would always be undefined. Instead, shouldn't it push moduleRaid.mObj[mKey] to the array, adding the module itself? In my tests this has worked...

I was going to do a PR with this, but reading the README, it states the following for what findFunction is supposed to do:

findFunction(query): Return functions that include query (query can be either a string or a function)

So is this not supposed to return the module itself? If so, how would one execute a function to custom match modules and get the result, as I suggested above?

Thanks in advance.

pixeldesu commented 5 years ago

@pedroslopez Sorry for the late reply, I was not watching my own repository apparently 🤔

I just checked the code, and you are right indeed. If the module keys are not number-based indices this will just find nothing. Good find!

I mainly use the string-based search, but good catch on this issue. If you want to, feel free to make a PR to fix this, I'll check and merge it ASAP!

Thank you very much! 👍

pedroslopez commented 5 years ago

Hmm... My first instinct to resolve the issue I was personally having was to simply push the module itself to the array, as stated in the initial post. However when reading your reply to this issue again while creating the PR, I saw the following:

If the module keys are not number-based indices this will just find nothing

Looking at the code, moduleRaid.mObj[index] is used when searching by string, and you @pixeldesu state that this is what you use, meaning it works for you. With my particular use case, however, the module keys are string based and I don't have an example where they are number based, so I don't really know how to test the other case.

This would mean that, while this works for my case, it probably does not solve the real issue. I'm not sure what the right answer is here, mainly because I don't have the other use case to test with; perhaps some tests should be added to define how moduleRaid is supposed to work under different conditions, or seeing a working example I could check it out.

pixeldesu commented 5 years ago

Ah, I was confused about my own code a bit right there (it has been a while too, not really touching it for 2 years). the cArr used by moduleRaid in this step is just an array, not an object with the string-based keys that are used in the mObj (or modules which are later returned)

Just now I noticed how wrongly I implemented this function.

because the string based search in findFunction uses the constructor-array correctly (turning the whole function into a string and searching for the string you pass to it in each function)

and the function based one, which actually should get the constructor get passed to user-made function which then validates the module, doesn't pass the constructor to the user-function, but a module from the module object.

I should definitely look into fixing that.

pedroslopez commented 5 years ago

Ah, I see. So there is not a way to search for modules based on a specified function? I guess what I'm really looking for here is to be able to pass a query function to findModule to do more complex searches...

Would you be interested in a PR that implements this?

pixeldesu commented 5 years ago

Sure, feel free to do that!

pixeldesu commented 4 years ago

@pedroslopez this change is now released, sorry that it took so long. I saw you made a fork in the meantime to get your changes published.

Newest release also includes non-manual builds, so if new stuff comes up I'll get to make releases quicker!