WebpackModules rewrite + remove external moment.js dependency #28
Conversation
| @@ -42,7 +42,7 @@ if (window.BetterDiscord) { | |||
| 'vendor': { | |||
| jQuery: require('jquery'), | |||
| $: require('jquery'), | |||
| moment: require('moment') | |||
| moment: window.wpm.getModuleByNameSync('Moment') | |||
There was a problem hiding this comment.
It's better to use local scope requires (WebpackModules) instead of global scope variables (window.wpm) as much as possible. To be honest I don't understand why we need window.wpm at all
There was a problem hiding this comment.
Yep nothing will be left in the window namespace in the end. It's there for debugging at the moment.
| const component = selector(module); | ||
| if (!component) return false; | ||
| if (!component.prototype) return false; | ||
| for (const field of fields) { |
There was a problem hiding this comment.
We can use fields.every here as well.
There was a problem hiding this comment.
Fair point. I'll change that for this and the other one you mentioned
|
|
||
| static combine(...filters) { | ||
| return module => { | ||
| for (const filter of filters) { |
| OnlineWatcher: Filters.byProperties(['isOnline']), | ||
| CurrentUserIdle: Filters.byProperties(['getIdleTime']), | ||
| RelationshipStore: Filters.byProperties(['isBlocked']), | ||
| MentionStore: Filters.byProperties(["getMentions"]), |
There was a problem hiding this comment.
What about UserSettingsStore?
UserSettingsStore: Filters.byProperties(['developerMode', 'locale']);
There was a problem hiding this comment.
Ah that's a better name haha, I have it in the list as CurrentUserState but it is the same object
| ContextMenuItem: Filters.byCode(/\.label\b.*\.hint\b.*\.action\b/), | ||
|
|
||
| /* In-Message Links */ | ||
| ExternalLink: Filters.byCode(/\.trusted\b/) |
There was a problem hiding this comment.
I like the idea of such list, but keeping unused stuff is dangerous. We definitely need to develop some kind of integration tests, to know when something from this list stops working on canary.
There was a problem hiding this comment.
Yes we will definitely need some tests and such to see when things stop working. But I believe plugins that want to rely on it should check if the return is null so they don't error out
There was a problem hiding this comment.
BTW, it might be the case of async versions. They might reject instead of resolve with null if there is no such module. But this isn't the case in current implementation. Btw look at new comment below
| const { exports } = module; | ||
| static getModuleByDisplayNameSync(name) { | ||
| return this.getModuleSync(Filters.byDisplayName(name), true); | ||
| } |
There was a problem hiding this comment.
This is almost useless. AFAICS most of react components already either don't have DisplayName or aren't exported from it's modules.
There was a problem hiding this comment.
There are still some with display name such as emoji picker, and several popouts
| return Cache[name] = await this.getModule(fallback, first); | ||
| } | ||
|
|
||
| static async getModuleByDisplayNameSync(name) { |
| return await this.getModule(Filters.byDisplayName(name), true); | ||
| } | ||
|
|
||
| static async getModuleByRegexSync(regex, first = true) { |
| @@ -99,6 +224,49 @@ class WebpackModules { | |||
| return __webpack_require__.c; | |||
| } | |||
|
|
|||
| /* Asynchronous */ | |||
There was a problem hiding this comment.
Why do we even need Async and Sync versions? Isn't webpackJsonp itself synchronous? AFAICS we won't get any benefit from making in looks like asynchronous.
There was a problem hiding this comment.
I agree the asynchronous won't have much benefit. I just did it because Jiiks had both in the original version so I assumed they wanted both in the rewrite
| } | ||
| return first ? rm[0].exports : rm; | ||
| return rm; |
There was a problem hiding this comment.
Is it intentional that we will return empty array if no such module found, regardless of first parameter?
There was a problem hiding this comment.
Good catch, I will fix this as well
|
Yes I know but I wasn't sure it was complete yet |
This is for #11
npm run gulp babelwithout a global gulp install