You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A complete refactor of the Vencord Webpack API. This rewrites how Webpack finds and module patches are done.
Changes to Webpack finding:
Completely deprecates the lazy way that we currently do Webpack finds. Instead, now the default will be a unified way which is a combination of the plain cacheFind for already loaded modules, and usage of waitFor for when modules were not yet loaded.
The way this works is:
If you use the find methods in the top level, you will leverage the performance gain of using the waitFor alternative. By using waitFor, everytime a new module is loaded, Vencord will check if that module matches any of the finds requested, and if it does, it sets the result of that find to the module. This avoids having to iterate through the module cache for every find.
By using the waitFor alternative, the result of calling the find methods is a Proxy, which has a value inside which points to the module. When the module is found that value is set, and now the Proxy acts as the module found.
Trying to access a Proxy value before the module value is set will cause an error, which shows the filter that failed. This allows users to post better logs for us.
However, if you don't use in the top level, you will mostly likely just find the module using cacheFind, which does need to iterate through the module cache. But of course, if the module is not in the cache it will still register a waitFor, to find the module in case it is loaded later.
By having the unified way, we no longer need a lazy alternative of the methods, so the following changes to the API occurred:
findModuleFactory now uses the same tactic as the normal finds, and its old version using cache is now cacheFindModuleFactory
Warning
Destructuring Webpack finds at top level is now deprecated. It used a very hacky tricky which turns out to not always have the expected result (pshhhh Firefox)
mapMangledModule is the only exception for this
Consider switching to mapMangledModule or findProp
Important additions:
mapMangledModule can now properly find and wrap components, simply use a component filter in one of its mappings
Additional:
Lots of cleanups related to the main Webpack finding API and lazy stuff, and some plugins practices regarding modules
Webpack commons and a lot of plugins finds were changed for more stability and clean code
Mildly refactor to reporter Webpack find testing, taking advantage of the way the new finds work, so it no longer has to re-search everything
As stated before, better error for modules not found
Migrated all plugins to use the new settings API
Important
This change is fully backwards compatible for now. Old methods were kept and just point to the new APIs and top level destructuring is kept with a warning.
The plan is to remove the deprecated methods/features in the future.
Changes to Webpack module patching:
Centralizes Webpack patching around a single thing: Patching the module factories object of the WebpackRequire (wreq.m). This wraps the modules object with a proxy to intercept the addition of new modules, and allow us to decide what to do with them.
A new internal setting was added, eagerPatches, and it decides what to do with the newly added modules. With it enabled, we patch the factories as soon as they are set, and with it disabled we only patch them once they are accessed for the first time (default).
For correctly handling patching, because of multiple WebpackInstances, and to allow for patching factories only when they are accessed, our proxy defines getters and setters for each module factory added, instead of doing a simple Reflect.set on the module factories object.
Additionally, these were done:
Patch timings testing! Reporter now says if a patch took a long time to complete (we don't want slow patches)
A lot of clean up
Harder, yet stable conditions for choosing what to do with the WebpackInstances found, and whether it is the main instance, used for Webpack finding
I analyzed years old instances of webpack to see what was less likely to change and rely on.
Complete typings and documentation of the WebpackRequire. All of the code now properly uses the correct typings.
A more future proof and memory efficient patchedFactory wrapper. It includes a fallback to set the main WebpackInstance in case our other approach somehow fails, and also keeps less things into its closure, allowing the garbage collector to act.
Small reporter refactor to fit the removal of the beforeInitListeners API
Add error handling for the asynchronous call to getText()
Consider handling the case where getText() might return a promise that could reject. Currently, if getText() throws an error, it will lead to an unhandled promise rejection. You can add a try-catch block around the await getText() call to handle potential errors gracefully.
Why: This suggestion is crucial as it addresses potential unhandled promise rejections, which can lead to runtime errors and crashes. Adding a try-catch block ensures that any errors from the getText() call are handled gracefully.
9
Add error handling to the chunk loading promises to prevent one failure from affecting others
Consider adding error handling for the wreq.e(id) call within the Promise.all to handle potential rejections that could cause the entire promise chain to fail. This will improve the robustness of the code by ensuring that one failing chunk load does not prevent other operations from continuing.
Why: Adding error handling to the chunk loading promises is crucial for robustness, ensuring that one failure does not halt the entire process. This is a significant improvement in error resilience.
9
Add error handling for the findByCode function to manage cases where no results are found
Ensure that the findByCode function is correctly typed to handle the case where no matching code is found. It's good practice to handle potential undefined or null values that might be returned if the search criteria do not match any entries.
-const createBotMessage = findByCode('username:"Clyde"');+const createBotMessage = findByCode('username:"Clyde"') ?? throw new Error('Bot message configuration not found.');
Suggestion importance[1-10]: 8
Why: This suggestion improves robustness by handling cases where findByCode might return undefined. This prevents potential runtime errors and ensures the code behaves as expected even when the search criteria do not match any entries.
8
Add null checks and default values for methods that might return undefined
It's important to handle the case where PluginManager.startDependenciesRecursive or PluginManager.startPlugin might return undefined or an unexpected result. Consider adding checks or default values to handle these cases more robustly.
Why: This suggestion enhances code robustness by adding null checks and default values, preventing potential issues when methods return undefined. However, the impact is slightly less critical compared to handling unhandled promise rejections.
7
Maintainability
Improve readability and robustness of component existence checks
Replace the complex conditional check with a more readable and maintainable approach using optional chaining and nullish coalescing operators.
Why: The use of optional chaining and nullish coalescing operators improves readability and robustness, making the code more maintainable and less prone to errors.
9
Use a more descriptive variable name for the factory parameter in the factoryListener function
To improve the maintainability and readability of the code, consider using a more descriptive variable name than factory for the factoryListener function parameter. A name like moduleFactory would provide more context about the type and purpose of the parameter.
Why: Using a more descriptive variable name enhances code readability and maintainability, though it is a minor improvement.
6
Possible bug
Ensure fact is a string before using replaceAll to avoid runtime errors
The replaceAll method might throw an error if fact is not a string. Ensure that fact is always a string before calling replaceAll on it, or handle cases where fact might not be a string.
Why: This suggestion is important as it prevents potential runtime errors by ensuring fact is a string before calling replaceAll. This adds a layer of safety and ensures the code does not break unexpectedly.
8
Ensure the module function exists before invoking it to avoid runtime errors
It's recommended to check the existence of wreq.m[id] before attempting to call wreq(id) to avoid potential runtime errors if id does not exist in wreq.m.
Why: Ensuring the module function exists before invoking it prevents potential runtime errors, which is a good practice for improving code reliability.
8
Best practice
Add safety checks before accessing properties of potentially undefined objects
Ensure consistency and safety by checking the existence of ClientThemesBackgroundStore before accessing its properties.
Why: Adding a safety check with optional chaining ensures that the code does not throw an error if ClientThemesBackgroundStore is undefined, improving robustness.
8
Define the regular expression as a constant to improve code readability and maintainability
Instead of using a regular expression directly in the matchAll method, consider defining it as a constant at the beginning of the module. This enhances readability and maintainability, especially if the regular expression is complex or used multiple times.
-for (const currentMatch of String(wreq.u).matchAll(/(?:"([\deE]+?)")|(?:([\deE]+?):)/g)) {+const CHUNK_ID_REGEX = /(?:"([\deE]+?)")|(?:([\deE]+?):)/g;+for (const currentMatch of String(wreq.u).matchAll(CHUNK_ID_REGEX)) {
Suggestion importance[1-10]: 7
Why: Defining the regular expression as a constant improves readability and maintainability, which is a good practice, though not critical.
7
Previous suggestions
Suggestions
Category
Suggestion
Score
Best practice
Replace string literal exceptions with descriptive error messages or custom error types
Replace the string literal thrown in the exception with a more meaningful error message or a custom error type. Throwing string literals can make error handling and debugging more difficult.
-throw "a rock at ben shapiro";+throw new Error("Descriptive error message or custom error type");
Suggestion importance[1-10]: 10
Why: Throwing string literals is a bad practice as it makes error handling and debugging more difficult. Using a descriptive error message or custom error type improves code maintainability and clarity.
10
Replace non-null assertions with specific types or handle potential null values explicitly
Avoid using non-null assertions (as any) as they bypass TypeScript's type checking. If the type is uncertain, consider using a more specific type or handling the undefined/null case explicitly.
-let result = null as any;+let result: SpecificType | null = null;
Suggestion importance[1-10]: 8
Why: Avoiding non-null assertions improves type safety and helps prevent runtime errors. Using a specific type or handling null values explicitly makes the code more robust and maintainable.
8
Rename BlobMask to BlobMaskComponent to follow React naming conventions
The BlobMask variable is imported using findExportedComponent which suggests that it is a React component. It is recommended to follow the React component naming convention by capitalizing the first letter of the component name.
Why: Following naming conventions is important for code readability and maintainability, especially in a collaborative environment. This suggestion aligns with best practices for React components.
8
Improve type safety and readability by using a more specific type for lastGuildId
Consider using a more specific type for lastGuildId instead of string | null. If lastGuildId is expected to hold only certain types of strings (e.g., UUIDs, specific identifiers), using a more specific type or a type alias could improve type safety and code readability.
-let lastGuildId = null as string | null;+type GuildId = string; // Define this type according to your specific use case+let lastGuildId: GuildId | null = null;
Suggestion importance[1-10]: 7
Why: The suggestion improves type safety and readability, which is beneficial for maintainability. However, it is not critical to the functionality of the code.
7
Improve the function name cacheFindAll to findAllWithCache for better clarity
Replace the cacheFindAll function with a more descriptive name that indicates its purpose and functionality, especially if it involves caching mechanisms.
Why: The suggestion enhances code clarity by providing a more descriptive function name, which helps in understanding the code better. However, it is a minor improvement and does not affect the functionality.
6
Rename setDecorationGridItem to setDecorationGridItemComponent to clarify its purpose
It is recommended to use a more descriptive name for the setDecorationGridItem function to clarify that it is a setter function. This enhances code readability and maintainability.
-export const setDecorationGridItem = v => DecorationGridItem = v;+export const setDecorationGridItemComponent = v => DecorationGridItem = v;
Suggestion importance[1-10]: 6
Why: The suggestion improves code readability by making the function's purpose clearer. This is a minor enhancement that aids in maintainability but does not impact the core functionality.
6
Performance
Replace eager loading methods with lazy loading equivalents to enhance performance
Replace the direct imports and usage of findByProps and findStore with their lazy counterparts to avoid potential performance issues due to eager loading. This is especially important in a plugin system where minimizing the initial load time can significantly affect the overall performance.
Why: The suggestion correctly identifies a potential performance issue with eager loading and proposes a valid solution by using lazy loading methods. This change can significantly improve the initial load time of the plugin, which is crucial in a plugin system.
9
Use a lazy loading approach for findByProps to improve module loading efficiency
Replace the direct use of findByProps with a lazy or asynchronous variant to ensure that the module is loaded only when necessary, which can help in reducing the initial load time and potentially avoid runtime errors due to the module not being ready.
Why: The suggestion to use lazy loading can improve performance by reducing initial load time. However, the current use of findByProps may be intentional for immediate availability, so the impact might be minor.
7
Possible bug
Correct potential typo in the findComponentByCode function argument
Ensure that the string argument for findComponentByCode does not contain a trailing comma unless it is intentional to match specific criteria. This could be a typo that might lead to unexpected behavior or errors.
Why: This suggestion addresses a potential typo that could lead to unexpected behavior or errors, making it a crucial fix for code correctness.
9
Possible issue
Add error handling for cacheFind to ensure stability
Consider handling the case where cacheFind returns null or an unexpected result to prevent runtime errors. Implement error checking or default value assignment.
Why: Adding error handling for cacheFind enhances code stability by preventing runtime errors due to null or unexpected results, which is important for robust code.
8
Maintainability
Refactor nested if-else conditions into separate functions or a switch-case structure for better readability
Refactor the nested if-else conditions into separate functions or use a switch-case structure to improve readability and maintainability.
-if (searchType === "webpackDependantLazy" || searchType === "webpackDependantLazyComponent") {- const [factory] = args;- result = factory();-} else if (searchType === "extractAndLoadChunks") {- const [code, matcher] = args;- const module = Vencord.Webpack.findModuleFactory(...code);- if (module) {- result = module.toString().match(Vencord.Util.canonicalizeMatch(matcher));- }-} else {- const findResult = args.shift();- if (findResult != null) {- if (findResult.$$vencordCallbackCalled != null && findResult.$$vencordCallbackCalled()) {- result = findResult;+switch (searchType) {+ case "webpackDependantLazy":+ case "webpackDependantLazyComponent":+ const [factory] = args;+ result = factory();+ break;+ case "extractAndLoadChunks":+ const [code, matcher] = args;+ const module = Vencord.Webpack.findModuleFactory(...code);+ if (module) {+ result = module.toString().match(Vencord.Util.canonicalizeMatch(matcher));
}
- if (findResult[Vencord.Util.proxyInnerGet] != null) {- result = findResult[Vencord.Util.proxyInnerValue];+ break;+ default:+ const findResult = args.shift();+ if (findResult != null) {+ if (findResult.$$vencordCallbackCalled != null && findResult.$$vencordCallbackCalled()) {+ result = findResult;+ }+ if (findResult[Vencord.Util.proxyInnerGet] != null) {+ result = findResult[Vencord.Util.proxyInnerValue];+ }+ if (findResult.$$vencordInner != null) {+ result = findResult.$$vencordInner();+ }
}
- if (findResult.$$vencordInner != null) {- result = findResult.$$vencordInner();- }- }+ break;
}
Suggestion importance[1-10]: 7
Why: Refactoring nested if-else conditions into a switch-case structure improves readability and maintainability. It makes the code easier to understand and reduces the risk of errors.
7
Add fallback for findByProps to handle potential undefined properties safely
Replace the direct use of findByProps with a more specific method if available, or ensure that the properties being accessed are correctly encapsulated within the method to prevent accessing potentially undefined properties.
Why: The suggestion improves maintainability by ensuring that the code handles potential undefined properties safely, though the current implementation might already be sufficient in many cases.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Nuckyz
force-pushed
the
immediate-finds
branch
2 times, most recently
from
Vendicated
force-pushed
the
immediate-finds
branch
from
A complete refactor of the Vencord Webpack API. This rewrites how Webpack finds and module patches are done.
Changes to Webpack finding:
Completely deprecates the
lazyway that we currently do Webpack finds. Instead, now the default will be a unified way which is a combination of the plaincacheFindfor already loaded modules, and usage ofwaitForfor when modules were not yet loaded.The way this works is:
waitForalternative. By using waitFor, everytime a new module is loaded, Vencord will check if that module matches any of the finds requested, and if it does, it sets the result of that find to the module. This avoids having to iterate through the module cache for every find.By having the unified way, we no longer need a
lazyalternative of the methods, so the following changes to the API occurred:find->cacheFindfind(wrapper aroundwaitForandcacheFind)findLazy->find(wrapper)findByProps,findByPropsLazy->findByPropsfindByCode,findByCodeLazy->findByCodefindStore,findStoreLazy->findStorefindComponent,findComponentLazy->findComponentfindExportedComponent,findExportedComponentLazy->findExportedComponentfindComponentByCode,findComponentByCodeLazy->findComponentByCodemapMangledModule,mapMangledModuleLazy->mapMangledModuleThe following were added for additional filters and refactors purposes:
findProp-> Useful as an alternative to a top level destructure. See it's JSDoc documentation for more information.findComponentByFields-> Used for finding a class component using its prototype fieldsfindByFactoryCode-> Use for finding all the exports of a module using its factory codeThe following were changed for more appropriate names:
findAll->cacheFindAllfindBulk->cacheFindBulkfindModuleId->cacheFindModuleIdThe following were changed for less weird names:
proxyLazyWebpack->webpackDependantLazyLazyComponentWebpack->webpackDependantLazyComponentfindModuleFactorynow uses the same tactic as the normal finds, and its old version using cache is nowcacheFindModuleFactoryWarning
Destructuring Webpack finds at top level is now deprecated. It used a very hacky tricky which turns out to not always have the expected result (pshhhh Firefox)
mapMangledModule is the only exception for this
Consider switching to mapMangledModule or findProp
Important additions:
Additional:
Important
This change is fully backwards compatible for now. Old methods were kept and just point to the new APIs and top level destructuring is kept with a warning.
The plan is to remove the deprecated methods/features in the future.
Changes to Webpack module patching:
Centralizes Webpack patching around a single thing: Patching the module factories object of the WebpackRequire (wreq.m). This wraps the modules object with a proxy to intercept the addition of new modules, and allow us to decide what to do with them.
A new internal setting was added, eagerPatches, and it decides what to do with the newly added modules. With it enabled, we patch the factories as soon as they are set, and with it disabled we only patch them once they are accessed for the first time (default).
For correctly handling patching, because of multiple WebpackInstances, and to allow for patching factories only when they are accessed, our proxy defines getters and setters for each module factory added, instead of doing a simple Reflect.set on the module factories object.
Additionally, these were done: