rafaelgomesxyz / esgst

An extension that enhances SteamGifts / SteamTrades.
MIT License
146 stars 23 forks source link

Split code #896

Closed insideone closed 6 years ago

insideone commented 6 years ago

Hi, revilheart!

Would you accept PR with code spliting changes? I want to create builder based on webpack. The result file be identical, but we will get splitted project which is much easier to maintain collectively.

rafaelgomesxyz commented 6 years ago

Well, I just recently split the features: https://github.com/revilheart/ESGST/tree/master/Extension/Modules And I plan on splitting more stuff soon.

But if you think using webpack is easier than my method, then go for it. I'm not familiar with that tool.

insideone commented 6 years ago

Got it. I'll try.

I think using standardized module-system is most preferable. And this brings some bonus benefits like babel, auto-prefixers etc

rafaelgomesxyz commented 6 years ago

Ok, I'll close this issue and wait for the PR then. If you want I can help to speed up the process, just tell me what to do.

rafaelgomesxyz commented 5 years ago

Hey, did you do something already? I'm interested in working on this.

insideone commented 5 years ago

Yup, but it's early WIP :-( https://github.com/insideone/ESGST/tree/896/webpack

Preparing:

After that we can do project build via cmd execute ./node_modules/.bin/webpack


What I did:

TODO:

rafaelgomesxyz commented 5 years ago

Not sure if you're using a special loader, but I can't build the project because of some errors (for example, months is not defined inside of the constructor in Utils). Was that intended (because you're using a special loader that takes care of it) or really an error?

insideone commented 5 years ago

omg. I planned to add babel and some its presets, but totally forgot about it. Just fixed it on 21549b4, now it builds

clean-webpack-plugin: ~/projects/js/ESGST/build has been removed.
clean-webpack-plugin: 2 file(s) excluded - .gitignore, .
Hash: ac648b665b6a9ddb98ba
Version: webpack 4.16.5
Time: 39508ms
Built at: 2018-09-16 14:23:17
                  Asset      Size  Chunks             Chunk Names
monkey/ESGST.user.js.js  3.81 MiB       0  [emitted]  monkey/ESGST.user.js
Entrypoint monkey/ESGST.user.js = monkey/ESGST.user.js.js
  [0] multi ./monkey/index.js 28 bytes {0} [built]
  [1] ./monkey/index.js 20 bytes {0} [built]
  [2] ../main.js 104 KiB {0} [built]
  [3] ../lib/jsUtils/index.js 4.74 KiB {0} [built]
  [4] ../class/Esgst.js 30.5 KiB {0} [built]
  [5] ../lib/parsedown/index.js 35 KiB {0} [built]
  [6] ../modules/index.js 26.9 KiB {0} [built]
  [9] ../class/Popup.js 6.52 KiB {0} [built]
 [11] ../modules/Giveaways/BlacklistGiveawayLoader.js 9.29 KiB {0} [built]
 [13] ../modules/General/NarrowSidebar.js 3.52 KiB {0} [built]
 [14] ../modules/General/HiddenCommunityPoll.js 3.95 KiB {0} [built]
[159] ../modules/EndlessLoad.js 4.56 KiB {0} [built]
[160] ../modules/General/QuickInboxView.js 17.5 KiB {0} [built]
[161] ../modules/General/EndlessScrolling.js 53.4 KiB {0} [built]
[162] ../modules/Tags.js 66.5 KiB {0} [built]
    + 148 hidden modules

You need to repeat npm install because package.json is changed

rafaelgomesxyz commented 5 years ago

Hi, I forked the branch and made a commit (https://github.com/gsrafael01/ESGST/commit/a0affda049c17d5dab2bf020043e188661e67854) fixing the Comments modules. I hope I'm not doing things that you already did (maybe we could separate what modules each should work on). I'm going alphabetically, so I'm working on the Discussions modules next. And if you've made more changes it would be nice if you could commit them so we would stay in sync.

Also, there's no need to prefix functions like setValue and getValue, right? I'm new to this modules system, so I'm still figuring out how things work. Is this.esgst available for all modules and all classes? For example, is it available at class Popup or would I need to import another file there?

rafaelgomesxyz commented 5 years ago

Discussions modules fixed, now moving on to Games modules.

rafaelgomesxyz commented 5 years ago

Games modules fixed, now moving on to General modules.

rafaelgomesxyz commented 5 years ago

General modules fixed, now moving on to Giveaways modules.

insideone commented 5 years ago

Is this.esgst available for all modules and all classes?

Oops. I added it just now in Esgst.constructor

    for (let module of this.modules) {
      module.esgst = this;
    }

(cffbacb)

Also, there's no need to prefix functions like setValue and getValue, right? For example, is it available at class Popup or would I need to import another file there?

Maybe at first it seems awkward but any variable which you want to use needs to be imported beforehand. The point is to clearly control all dependencies. So, we need to import Popup and similar classes/objects in any module.

set/getValue need to be refactored because they are not exported now, so they can't be imported as well. The easiest way that I see it is dynamically inject this functions into esgst object or some module (common?), so it will be available in any module.


Also I want to add notice about this "trick":

import {utils} from '../../lib/jsUtils';
import {common} from "../Common";
const
  {getLocalValue, createElements} = common,
  {formatDate} = utils;

This is how you can shorten your code instead of using this.esgst.modules.common.getLocalValue or something like this. So basicaly we don't need to use this.esgst in modules. Fortunately such things are quite easily made by mass replacement


Planned to work on Groups, Trades and Users modules right now. I will report here when I finish

rafaelgomesxyz commented 5 years ago

If you compare the file from the webpack branch with the file from the master branch (I use Visual Studio Code for this), it's very easy to spot what's wrong. And also to add any changes that were made to the master branch since you started working on this.

insideone commented 5 years ago

I planned to add new functionality (made by you after I forked repo) after all converts to separate these two different tasks. Just pushed (7f1ce46) where I included environment functions in common module.

rafaelgomesxyz commented 5 years ago

Oh ok. Well, I'm already adding the new functionality because I can do it all in one go and it saves some time. I just finished the Giveaways modules. I'll sync my branch with yours and then commit it. I'm also writing a script to do the "trick" you mentioned on all of the files I've already fixed.

rafaelgomesxyz commented 5 years ago

https://github.com/gsrafael01/ESGST/commit/7071932b493e1d1d1e8817ba44ad62021d58ca8a does the shortcut thing. Probably needs some tweaks, because I did it automatically through a script. I'm currently giving it a quick look to see if it's all correct.

rafaelgomesxyz commented 5 years ago

The errors that I detected I fixed in https://github.com/gsrafael01/ESGST/commit/3bbe1a150eb2b4afd954910e6fc5935e6eff3af3

rafaelgomesxyz commented 5 years ago

Added shortcuts for environment functions as well (for the modules I fixed), and some other minor fixes: https://github.com/gsrafael01/ESGST/commit/99f3b0459aa5ba3481f8fc31042445e548015ba7

rafaelgomesxyz commented 5 years ago

Used your logic for environment functions to add environment variables for _USER_INFO, browser and gm: https://github.com/gsrafael01/ESGST/commit/7286c9e316b5b21a754740faecd4f15e7ade360c Hope I did everything correctly.

Are you still working on the Groups, Trades and Users modules? I can take one of those if you want to help speed things up. I was going to fix the Common module next, if you're not working on it.

insideone commented 5 years ago

Used your logic for environment functions to add environment variables for _USER_INFO, browser and gm: 7286c9e Hope I did everything correctly.

Looking good

Are you still working on the Groups, Trades and Users modules? I can take one of those if you want to help speed things up. I was going to fix the Common module next, if you're not working on it.

It seems like I will finish only Users modules today. Also I already fixed part of Common module, so I will end with it soon So you can fix other modules

rafaelgomesxyz commented 5 years ago

Are you only fixing them or defining types as well?

insideone commented 5 years ago

I define types just when it's needed to fix errors

rafaelgomesxyz commented 5 years ago

Fixed Groups, Trades and some other modules: https://github.com/gsrafael01/ESGST/commit/95baf2fe2c11eafe7386362e461159b229e74aff

Do you know if there's a way that I can get the "Unresolved Javascript variable" error on Visual Studio Code? Because I don't get the same errors that you do, so our modules will be out of sync.

insideone commented 5 years ago

Pushed User modules and common module fixes (ad32c41) Please pay attention to comments that start with --?


The last time I worked with Visual Studio was about 15 years ago, so I can't say something useful :-( It was even problematic to install it, because I'm a linux user.

Well, the point is that JavaScript is a very dynamic language, so in many cases IDE has to guess where the field is created, each IDE can do it on its own.

But I don't think that this is a big problem. Despite the fact that we have different IDE we can always write such code that will satisfy both systems and will look good.

rafaelgomesxyz commented 5 years ago

Oh, I just pushed another one as well, this time applying fixes to errors detected by Webstorm: https://github.com/gsrafael01/ESGST/commit/061957bb81efffeb332dea495bfe061824ba4b9e I'll sync with yours in a minute.

I never had any problems installing Visual Studio Code, and I'm a Linux user as well.

But I just ended up installing Webstorm, it looks good so far.

insideone commented 5 years ago

I never had any problems installing Visual Studio Code, and I'm a Linux user as well.

My bad, I was not attentive. Visual Studio Code is for sure available for linux, but I thought about Visual Studio. But Visual Studio Code is not an IDE (according to description), so inspections can be simplified.

btw if you use linux, why do you use mergeModules.ps1? It's PowerShell file, right?

rafaelgomesxyz commented 5 years ago

Yeah, Code is just an editor. Anyways, I can use Webstorm with no problems.

I have 2 HDs, one has Windows and the other Linux. I prefer Linux, but unfortunately some games don't run well, so I'm forced to keep a Windows HD.

I was on Windows when I was working on the script back then, but now I'm on Linux. If you take a look at the master branch you will see that it now has the .sh versions of those files: https://github.com/gsrafael01/ESGST/blob/master/mergeModules.sh

rafaelgomesxyz commented 5 years ago

I forgot to ask if you were still working on it, so sorry if I did something that you've already done. I just pushed https://github.com/gsrafael01/ESGST/commit/ad00d206b94b5571d08cc340c149c981845fe7c0 fixing some stuff. I haven't finished yet, so there's more coming later on.

rafaelgomesxyz commented 5 years ago

And another one: https://github.com/gsrafael01/ESGST/commit/c6a309a387bae35b252ca4ef7ae3b753b90b4314 I think most of the changes are done. I only need to check main.js next.

Also, how can we use the JSZip library? Do we have to import it from somewhere?

rafaelgomesxyz commented 5 years ago

Final one for today: https://github.com/gsrafael01/ESGST/commit/aeca2e64eb2fdf41fc6a181a2a82888b9ac1b133

insideone commented 5 years ago

Also, how can we use the JSZip library? Do we have to import it from somewhere?

You need import any external lib fetched by npm in any file where you plan to use it. More relative info: https://webpack.js.org/api/module-methods/ https://webpack.js.org/concepts/module-resolution/


It's funny, without ESLint IDE don't show any errors about JSZip in common module, so I didn't notice it. I want to revisit common + users modules now.

insideone commented 5 years ago

Pushed: 8231ebb (detailed description in the commit) btw you can run build from here: image


Now we need to decide how to deal with these

// @require https://raw.githubusercontent.com/revilheart/ESGST/7.26.3/Extension/js/jquery-3.3.1.min.js
// @require https://raw.githubusercontent.com/revilheart/ESGST/7.26.3/Extension/js/jquery-ui-1.12.1.min.js
// @require https://raw.githubusercontent.com/revilheart/ESGST/7.26.3/Extension/js/bootstrap-3.3.7.min.js
// @require https://raw.githubusercontent.com/revilheart/ESGST/7.26.3/Extension/js/interact-1.3.4.min.js
// @require https://raw.githubusercontent.com/revilheart/ESGST/7.26.3/Extension/js/query-builder-2.5.2.min.js
// @require https://raw.githubusercontent.com/revilheart/ESGST/7.26.3/Extension/js/intersection-observer.js
// @require https://raw.githubusercontent.com/revilheart/ESGST/7.26.3/Extension/js/encoding.js
// @resource bs https://raw.githubusercontent.com/revilheart/ESGST/7.26.3/Extension/css/bootstrap-3.3.7.min.css
// @resource abc https://raw.githubusercontent.com/revilheart/ESGST/7.26.3/Extension/css/awesome-bootstrap-checkbox-0.3.7.min.css
// @resource qb https://raw.githubusercontent.com/revilheart/ESGST/7.26.3/Extension/css/query-builder-2.5.2.min.css
// @resource sg https://raw.githubusercontent.com/revilheart/ESGST/7.26.3/Extension/css/steamgifts-v34.min.css

They can be easily included in build Can I do this?

insideone commented 5 years ago

It seems like ESGST.meta.js is important file for GM, right? It's possible to rebuild it each webpack build, so changing in two different places is not needed

rafaelgomesxyz commented 5 years ago

Sure, you can do it.

Yes, it's important. And it would be nice if the .meta.js and .user.js files were built in the main directory, because users already get updates from there. If we move its location, I think users will have to update manually.

As for the warnings you disabled, I already fixed all of the unfiltered for .. in loops anyway (or at least I think I did), and I'd think that's the safe way to do it, isn't it? And the typo warning is actually useful, I found tons of serious typos with it.

rafaelgomesxyz commented 5 years ago

Can I can use <% package.version %> anywhere?

insideone commented 5 years ago

And it would be nice if the .meta.js and .user.js files were built in the main directory, because users already get updates from there. If we move its location, I think users will have to update manually.

Got it. I'll try it today

As for the warnings you disabled, I already fixed all of the unfiltered for .. in loops anyway (or at least I think I did), and I'd think that's the safe way to do it, isn't it? And the typo warning is actually useful, I found tons of serious typos with it.

Typo inspection is helpful, but it generates a lot of false positives. Though if the dictionary will be full then it will be ok. So, let's try turn it on. "for .. in" - maybe I shouldn't have turned it off, but parsedown need 5 fixes (that's will be ok I think)

Can I can use <% package.version %> anywhere?

Sadly, no. banner.js got this feature because of this code in webpack.config.js:

return calfinated.process(fs.readFileSync(bannerFile, 'utf8'), {package: packageJson});

I think that it's possible to implement custom loader but I'm not familiar with this. But maybe it will be enough: https://webpack.js.org/plugins/define-plugin/

rafaelgomesxyz commented 5 years ago

You're working only on building the userscript files, right? I'll be working building the extension files.

insideone commented 5 years ago

Wait a sec. I faced circular dependency problem and prepare a fix

insideone commented 5 years ago

I faced the problem. Trace (from start to end):

main.js

import {common} from "./modules/Common";

Common.js

import ButtonSet from '../class/ButtonSet';

ButtonSet.js

import {common} from '../modules/Common';

const
  {
    createElements
  } = common
;

As you can see, button addresses to common module while it's not exported yet and the result of this is equal to null.

This circular dependency must be resolved. I implemented one of the ways to fix it in (b13af61)

insideone commented 5 years ago

You can continue any editing. I can go further only after a day

rafaelgomesxyz commented 5 years ago

Yeah, I noticed there was a problem because I built the extension file to test it and it didn't work. Thanks for the fix.

I can go further only after a day

What do you mean?

insideone commented 5 years ago

What do you mean?

I don't have free time until 18:00 GMT

rafaelgomesxyz commented 5 years ago

https://github.com/gsrafael01/ESGST/commit/84929dd0b161ef0315c4e7277a28bb289aa3b7fd

I'm getting an error here when trying to test the extension, saying that _this is undefined in this line: https://github.com/gsrafael01/ESGST/blob/896/webpack/src/main.js#L93

I've been trying to fix it, but haven't figured out how yet. One of the things that I tried was replacing () with .call(this); in the main.js file. No idea if I had the right idea. xD Do you know how to fix it?

insideone commented 5 years ago

Checkout feadc17 for fixes But now we have Uncaught (in promise) TypeError: common.isSet is not a function Working on it

insideone commented 5 years ago

c17406f - that works without any console errors: image But I think there're many hidden ones.

I continue working on https://github.com/gsrafael01/ESGST/issues/896#issuecomment-423809585

insideone commented 5 years ago

Where do you use interactjs? And bootstrap-3.3.7 - it's a custom build of bootstrap? Another WIP here: 1414b0b

rafaelgomesxyz commented 5 years ago

I don't remember, let me check. I think there were a few libraries that I had to modify in order to work well with the script, I'll check.

rafaelgomesxyz commented 5 years ago

interact.js 1.3.4 is a dependency for the sortable plugin in jQuery QueryBuilder: https://querybuilder.js.org/plugins.html#sortable

Awesome Bootstrap Checkbox 0.3.7 is a dependency for the bt-checkbox plugin in jQuery QueryBuilder: https://querybuilder.js.org/plugins.html#bt-checkbox

The .css file for jQuery QueryBuilder was modified to contain some extra styling for ESGST, but I guess I can move those to esgst.style. In fact, I think that would be the right thing to do.

Bootstrap Select 1.12.4 used to be a dependency for the bt-selectpicker plugin in jQuery QueryBuilder: https://querybuilder.js.org/plugins.html#bt-selectpicker

And Bootstrap 3.3.7 used to be a dependency for Bootstrap Select 1.12.4, because it didn't support Bootstrap 4. But that plugin has been gone for a long time now, because it was slowing things down, so I think it's safe to remove all Bootstrap and Bootstrap Select files.

JSZip 3.1.5 had to be modified because users could not export .zip files on Firefox (#781). This piece of code had to be added to the beginning of it:

ArrayBuffer = window.ArrayBuffer;
Uint8Array = window.Uint8Array;
Uint16Array = window.Uint16Array;
Int32Array = window.Int32Array;
Uint32Array = window.Uint32Array;

Not sure if that happens with the current scenario, I'll have to test.

rafaelgomesxyz commented 5 years ago

encoding.js is a polyfill for browsers that do not support TextEncoder, which is used in Common.js.

rafaelgomesxyz commented 5 years ago

It appears that when createElements is called from wbc_addButton (https://github.com/gsrafael01/ESGST/blob/1851d45cceed72a3a0f4bd81c251096ff8b65f34/src/modules/Users/WhitelistBlacklistChecker.js#L129) after clicking on the ESGST button at the header, this is undefined inside of the method (https://github.com/gsrafael01/ESGST/blob/1851d45cceed72a3a0f4bd81c251096ff8b65f34/src/modules/Common.js#L14621). Really strange, I can't figure out why yet.