Migrate to JavaScript/TypeScript #3341
Conversation
|
Based of you to do this. I cloned the repo and build the Userscirpt my self I found out that alot of the bugs are due to the type of If you look at the code with fx violentmonkey you can see it for your self see pic rel I will try to look trough the files and try to fix them, but it might be a little difficult due to my own fork. ( I think i will just make a burner/second acc ) As for all the things that you find strange, weird, etc like the Makefile, "Json test thing" , etc is that you just delete or rewrite it. I mean it uses Coffee script, a language that only shows 12 projects when searching it up on Github most of them being left to rot, the reason for the rewrite is to remove all the obscure shit and bloat |
Seems to be a problem with violentmonkeys syntax highlighting. Doesn't happen when in VS Code, or in GitHub markdown: $$1.extend(link, {
innerHTML:
`<a href="javascript:;" class="qr-link">${g.VIEW === "thread" ? "Reply to Thread" : "Start a Thread"}</a>`
});Having an unclosed |
|
I have tried building this myself and i have found a bug: I will look and try to fix this at this myself, I will report back if i fix it. |
|
Fixed it Maybe you all ready know this, but when bug fixing try to look at the original 4chan-X build files, that is what helped me here. |
The most important test is checking if the HTML built from JSON matches 4chan's HTML. This should probably be rewritten to happen in a script running in Node that imports a few relevant modules instead of in a special build of the userscript. |
Thanks, can you open a to my fork? Preferably the 'project-XT' branch.
That's probably important now that I changed the html generation. I'll see if I can put it in a separate script. |
|
The test rely on the dom, same with the things it's testing. Making them a node script would not only mean rewriting the tests, but a lot of the script as well. Ran the tests. Only differences with the old template output was a few spaces between elements. I changed the script to make the tests pass. |
|
@TuxedoTako Also @ccd0 i think a history/log wipe is needed after this merge ( An archive can be be but elsewhere) I think its because of multiple years of having multiple build files (aka user scripts ) in the source code Tagging is a lot better option and github supports having binaries/files on tags under releases. or at least only have one file instead of 3. The history is also no longer important due to the language change |
|
@LalleSX |
|
@LalleSX I asked for a PR of the fix, I didn't expect you to start working on the build script, moving some files, and emptying the build directory. For that I have to ask @ccd0 If he's ok with that, since those changes will also land in this PR to his repo. Now that I'm pinging anyway, are there more documented behaviors of 4chan-X for other scripts extending it than the events listen on the GitHub wiki? I don't know what needs to be added to the global window to make other scripts work, like 4chan sounds player, which I use myself and doesn't work with the fork. Or is this a case of scripts relying on observed behaviour, meaning things like replacing |
|
I would assume the switch to TS is one PR, and the changing of make/build is another, and the history stuff/deleting old builds is another. I think most userscripts utilise the API provided in the wiki along with things like IndexRefresh and IndexBuild. Otherwise personally I use MutationObservers in addition for when things don’t fire as you expect. There is an issue somewhere about adding more like PostsAdded as an array within threads. |
|
As saxamaphone69 said, breaking this stuff into multiple PRs makes sense. Also it looks like @LalleSX deleted a bunch of stuff from the Makefile but didn't write replacement functionality, mainly the stuff about updating the webpage and pushing builds to 4chan-x.net. That stuff would need to be replaced. For security reasons, the only way userscripts can communicate each other is by firing events or updating the page. So there's no need to worry about replacing |
Because the old build script expects coffeescript, it can't build the new version, so I had to make a new build script along with the migration to make sure it runs.
It was never my intention to put that in this PR.
Yep, like I mentioned in the op or the readme of my fork, I wanted to at least keep the old build scripts until the new one can do everything the old one did. Sorry Lalle, but I can't merge a PR that removes old functionality without writing a new alternative. Especially since the script that updates the web page might not even need a rewrite. If I thought just outputting an userscript and extension directory was enough this PR would be out of draft.
I should look deeper into the 4chan sounds player script and what is actually breaking it. |
|
Looks like I may need to rewrite some of the build and distribution stuff prior to this pull request being merged just in order for contributors to make releases. Will post more details in #3339. |
|
It may be some time before we stop updating the CoffeeScript version, so manually porting the code might not be sustainable. Have you tried instead running decaffeinate on the updated version, then merging that with your branch? |
This isn't actually the case. If you just change the names to .js, they should work with the old build script. A few files are already in Javascript. |
Decaffeinate didn't add the imports and exports, that was me. So a new decaffeinate will be merge conflict hell. I already ported up to release v1.14.22.4, and I'm willing to continue that for a while if the changes aren't too big.
Looks like my new import/export version broke that... import/export was needed for TypeScript anyway, not just for .ts files, but for nice IDE functions like going to definition and looking up references. The alternative is declaring globals manually. Anyway, I have trouble understanding the Makefile, and since it apparently does more than building the script, it seems that it can't be replaced with my |
|
Found one problem with the 4chan sound player: https://github.com/rcc11/4chan-sounds-player/blob/f2953446fd9403872642e05f91f580aab148ec44/src/main.js#L35. Because it's designed to work with both 4chan-X and native, it also listens on DOMContentLoaded, which 4chan-X is also waiting for before initializing, resulting in a race condition. |
|
Is the An example/showcase of this working is this: function main() {
alert("There are " + $('a').length + " links on this page.");
}
main(); |
The bloat is not needed in modern browsers. It's just jQuery-like as in aliased in an easier-to-manage way and chainable. |
|
What options did you use for decaffeinate? |
|
seems to reproduce most of it except for commenting out the constructor in Post.Clone; also the suggestion "DS002: Fix invalid constructor" seems to have been removed. Was that done manually? |
Probably a good idea to split up the decaffeinate commit into two parts like this. That way it's still easy to look at the history of a particular file. Subsequent commits would be rebased on that. |
|
Regarding archive.json, that's something I periodically pull from https://github.com/4chenz/archives.json so it should be left as a plain JSON file if possible. Otherwise, there will need to be a conversion every time I pull it. |
|
Why does this commit |
Tried it myself, and even with the whitespace changes, merging doesn't seem any harder than porting the code, and is probably more reliable. (In your port of the settings updates, the regexp It could be a year before I switch the stable version from CoffeeScript to JavaScript, so it will probably be me merging commits at that point. It might also reduce diff noise to leave the |
|
I'm still not convinced the JSX thing is a good idea. It looks like it adds needless complexity relative to just using templates, and the fact that it's not quite standard JSX is another point against it. |
I forgot...
Yes, I wanted to at least get the code to run before creating a PR. I must admit I made other changes suggested by decaffeinate, like removing Array.from around things that were already arrays, and changing edits to class prototypes to static properties.
I did to much work outside the decaffeinate, like the imports/exports, build, fixes to get get that build running, etc, to do it again. I can try a rebase on top of that. Maybe squash some work in progress commits. What are your thoughts on rewriting the history of this PR?
Auto formatting. I didn't expect you to go trough this commit by commit, and the contributing guidelines don't specify formatting rules, so I don't expected that to be a problem. You can add
Fair. I started converting .json files to js at the start, but the wrote custom code to import the package.json and version.json, so I should probably do that for other files that were previously .json.
Weird, the auto formatter should know regexes.
Like I said, om not doing the decaffeinate a second time. Maybe I can revert those manually and squash those in the commit from the bulk decaffinate.
I wouldn't call the original src/site/SW.yotsuba.Build/PostInfo.html simple.
What do you mean standard JSX? React? Which version? There is a specification that facebook published: https://facebook.github.io/jsx/ that's just a guide on how to transpile it. I tried to stay close to html strings, like how I'm not changing attribute names in the functions. There was talk about tagged templates in a /g/ thread, but I don't think it would be much simpler, especially when porting what was |
Presumably most of that was in separate commits though? It looks like it.
If it makes things clearer, feel free to rewrite the history if you want to. If you can get rid of the whitespace changes in cca085e, that would be good. But another commit reverting them would also work.
You don't need to do it, I'll be doing it. So to avoid interfering with the merges I'm going to be doing, can you try to avoid unnecessary diff noise between the decaffeinate commit (which should be reproducible just by me running decaffeinate) and the final pull request? This would mean not including ported commits in the pull request. Oh, and please don't squash in even more changes with the bulk decaffeinate commit, that would make things much worse.
The old templating system has a lot of complexity, but that complexity isn't at runtime. If you could get the JSX stuff to compile to something simple at build time, instead of having to walk a tree of elements at runtime, that would be a lot more attractive.
I don't know much about JSX, but I presumed the main advantage of using it would be that people familiar with it will know how to use it without us having to explain it much. But it seems there are subtle differences like the class/className thing. Not really a dealbreaker, just a minor point.
Looks like you're dealing with it by pushing the items into an array, which seems reasonable. So the tagged template function just needs to be able to deal with array-valued placeholders, in addition to safe HTML and unsafe strings. Maybe I'll write it myself and we can see what the runtime performance is like compared to the JSX version. |
|
Went trough my commits, removed unnecessary changes like following the decaffeinate suggestions, and squashed them. I omitted the ports of the changes since the decaf. I still have them, but as I understand it you want to review the decaf first. Because this is closer to the raw decaf, it has some bugs:
This is wrong because classList has Seems this is decaffeinated from $.hasClass = (el, className) ->
className in el.classListWhich the original build script turns into $.hasClass = function(el, className) {
return indexOf.call(el.classList, className) >= 0;
};Looks like it might need some work from the Is it ok if I test the script and add fixes for things like this? I'll add them as separate commits. |
- cca085e start of import/export - f816da1 start of changing stuff until I can get a bundle - c92adde first bundle without errors - e652dd2 Bundling works with ts files - 60fdb25 meta info in compilation - 8ccae78 new build doesn't cause errors on page load as userscript - 6fa11c4 work in progress: load userscript in browser and fix bugs - b15c557 migrated yotsuba templates to plain js the old templates caused some variable be in a wrong scope after decaffeinate, causing them to be unreadable from the old template the old templates caused some variable be in a wrong scope after decaffeinate, causing them to be unreadable from the old template - 9d763e8 update readme - 924eda8 added more imports, and now the circular dependencies are haunting me - ddd2d23 jsx templates for escaped strings, more bug fixed from circular dependencies - fee484d some fixes, clarify jsx - e1d01d0 Unpacked extension more fixes - 97d9090 fixed class on post that caused catalog to appear empty - 96a2c7b A child class that's not supposed to run the parents constructor? That needs a workaround in es6 classes. - fc06b4e changed jsx to make the tests pass - 7b317b2 revert archive and banners to json
|
Thinking about the templating thing some more, probably the ideal thing would be to use JSX but have the build script do the tree walk at build time. The compiled script would either contain a bunch of string concatenation or possibly tagged templates, depending on how well the latter performs. |
Yes, that will be helpful. |
(cherry picked from commit 5815070)
(cherry picked from commit ebe35f2)
- Fix for unwanted sorting of catalog under certain settings. [ccd0#3212](ccd0#3212), 7dfba22 - Turn JS Whitelist functionality off by default. 419e90c - Better way of turning off JS Whitelist. 7df2750 - Update documentation. 62e4ccf - Fallback when XPCNativeWrapper is unavailable [ccd0#3430](ccd0#3430) - Add ability to clear whole thread watcher [ccd0#2926](ccd0#2926) (cherry picked from commit e900a0c) - Updated youtube regex - Updated archives list - 4channel.org domain is no more; fix issue with links not being bold in headers. dd01b1a - mp4 support #124, efed1ab
Fixes #829
Creating a draft PR because this is not entirely done, but I think it's far enough to discuss. There was already some discussion on /g/, but I think it's better to have the discussion here.
This migration is based on transpilation done with decaffeinate, with export/import syntax added. Bundling is now done with rollup, and can be done with
npm i; npm run build, and not with make anymore.I tried to keep it at close as possible to decaffinates output, but I had do do some things to make it compatible with the new build and get it running without errors, like finding an alternative to the old custom templates, which is now done with jsx.
This build is still mostly .js, but the build allows simply renaming files .js to .ts or .tsx except for build scripts. Typing every file would be an ever bigger project than this migration, so for the foreseeable future types can be added when needed or wanted.
notes
$.dict()and$.SECONDS"checkJs": true,, and a lot of js files report type errors when opened because of unknown properties on objects and reassigning variables with different types. These errors don't block the bundle at this moment.I don't really understand PostClone, so that might have bugs after migrationI think the main thing that confused me the the workaround coffeescript put in to not run the constructor of the parent class Post. A es class compatible workaround was implemented.Things that still need to be done:
add things to the global window for use by other userscriptsaccording to Migrate to JavaScript/TypeScript #3341 (comment), non global is intendedcommits since this was forked
Click to expand
nice to have in the future
.callto set thethisobject to passing variables. Unlessthisis explicitly typed, it won't be recognized by TypeScript and listed in references or when renaming variables across the project