[WIP] Disentangle JS modules #225
Conversation
|
|
||
| $(document).ajaxSend(ajaxSend); | ||
| $(document).ajaxComplete(ajaxComplete); | ||
| $(document).ajaxError(ajaxComplete); |
There was a problem hiding this comment.
'ajaxComplete' is not defined no-undef
| /* global config, persistChoices, iso639Codes, iso639CodesInverse, populateTranslationList, showTranslateWebpageInterface */ | ||
|
|
||
| $(document).ajaxSend(ajaxSend); | ||
| $(document).ajaxComplete(ajaxComplete); |
There was a problem hiding this comment.
'ajaxComplete' is not defined no-undef
| /* exported SPACE_KEY_CODE, ENTER_KEY_CODE, HTTP_OK_CODE, HTTP_BAD_REQUEST_CODE, XHR_LOADING, XHR_DONE */ | ||
| /* global config, persistChoices, iso639Codes, iso639CodesInverse, populateTranslationList, showTranslateWebpageInterface */ | ||
|
|
||
| $(document).ajaxSend(ajaxSend); |
There was a problem hiding this comment.
'ajaxSend' is not defined no-undef
|
The CircleCI tests claim that But this is not true, they are both exported and then imported by other modules. |
index.html.in
Outdated
| <script src="//oss.maxcdn.com/libs/html5shiv/3.7.0/html5shiv.js"></script> | ||
| <script src="//oss.maxcdn.com/libs/respond.js/1.4.2/respond.min.js"></script> | ||
| <![endif]--> | ||
| <script type="text/javascript" src="./js/init.js"></script> |
There was a problem hiding this comment.
Why was this added separately? Pretty certain it shouldn't be here.
There was a problem hiding this comment.
Sorry, needed a moment to figure out how things get loaded ...
assets/js/compat.js
Outdated
| }; | ||
| } | ||
|
|
||
| // From https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign#Polyfill |
There was a problem hiding this comment.
I think this shouldn't have been moved because compat.js only gets added for IEx-y versions but this shim is required for more than that.
There was a problem hiding this comment.
What about having two separate levels of compat?
Something like compat-all.js and compat-ie.js?
There was a problem hiding this comment.
Seems unnecessarily complicated. Just putting it into util.js with a comment as to why seems OK to me.
There was a problem hiding this comment.
Complicated? Seems perfectly reasonable to me ...
I mean, it doesn't really belong in util.js and neither in init.js, does it?
There was a problem hiding this comment.
It's unnecessarily complicated. If we start having multiple functions we polyfill for all browsers, then this makes sense. A file with literally one function is silly.
There was a problem hiding this comment.
Why don't we just put all polyfills inside one file in the first place?
I mean, it's not like we'd be shipping all too much unnecessarily.
There was a problem hiding this comment.
The app already ships more KB of JS than it should because we don't use proper minification. There's no point in making it worse than it currently is by giving everyone polyfills they don't need.
There was a problem hiding this comment.
Fine, but util.js is not ideal either. I'll move it back for now.
There was a problem hiding this comment.
It's not ideal but I think it's a good compromise between simplicity and performance.
I don't think they're exported from Also, |
They are, by this line: assets/js/util.js#L410
It's only ever used in |
|
The line you mention is used by Flow, not the linter. Check out: https://github.com/goavki/apertium-html-tools/blob/master/assets/js/util.js#L3
That's fine. My argument still holds :) |
Anyway, the CI tests fail if these functions are declared in |
No, they don't. Use the export comments correctly. |
|
Okay, now I got it. Will update the imports. |
assets/js/util.js
Outdated
| @@ -1,14 +1,13 @@ | |||
| /* @flow */ | |||
| /* exported sendEvent, modeEnabled, filterLangList, getURLParam, onlyUnique, isSubset, safeRetrieve, callApy, apyRequestTimeout, isURL */ | |||
| /* exported ajaxComplete, sendEvent, modeEnabled, filterLangList, getURLParam, onlyUnique, isSubset, synchronizeTextareaHeights, callApy, apyRequestTimeout, isURL */ | |||
There was a problem hiding this comment.
Line 2 exceeds the maximum line length of 140 max-len
|
No errors in console when running locally. |
|
I know @sushain97 thinks of having separate |
Makefile
Outdated
| debug: debugjs debugcss build/index.debug.html build/not-found.html fonts build/js/compat.js build/js/jquery.min.js build/js/bootstrap.min.js build/sitemap.xml build/strings/locales.json build/index.$(DEFAULT_LOCALE).html build/strings/$(DEFAULT_LOCALE).json images | ||
|
|
||
| prod: js css html fonts build/sitemap.xml build/manifest.json build/strings/locales.json localhtml images | ||
| prod: js flow css html fonts build/sitemap.xml build/manifest.json build/strings/locales.json localhtml images |
There was a problem hiding this comment.
I think this adds flow to the dependencies for building this project. We can't add any dependencies (for users).
There was a problem hiding this comment.
Shall we run make flow directly as part of CircleCI?
There was a problem hiding this comment.
Yeah :) before the make -j8 because that sometimes fails due to APy being down (hence the || true.
There was a problem hiding this comment.
Also, I might open another ticket for typing html-tools with flow if you're interested.
There was a problem hiding this comment.
Sure, put together the specs.
There was a problem hiding this comment.
Will look up the syntax and stuff while you are writing it up.
index.html.in
Outdated
| </script> | ||
|
|
||
| <!--[if lt IE 9]> <script type="text/javascript" src="./js/compat.js"></script> <![endif]--> | ||
| <!--[if lt IE 9]> <script type="text/javascript" src="./js/compat-ie.js"></script> <![endif]--> |
| ### Clean ### | ||
| clean: | ||
| rm -rf build/ | ||
|
|
There was a problem hiding this comment.
A single trailing line with some whitespace on it.
Did it serve any purpose?
There was a problem hiding this comment.
I guess it should have been a single trailing line without whitespace.
There was a problem hiding this comment.
Other files in the repo don't have trailing newlines either. Do we want a trailing newline here?
There was a problem hiding this comment.
Huh, I thought they all did. They all should! Don't worry about the other files but let's retain one here.
Makefile
Outdated
| flow: build/js/all.js | ||
| grep -Ev '/\*:: *(ex|im)port ' $< | flow check-contents | ||
| flow: $(JSFILES) | ||
| grep -Ev '/\*:: *(ex|im)port ' $< | npm run flow check-contents |
There was a problem hiding this comment.
Thought of that, too. This grep strips all lines containing the regex. When the comment has a line break, it keeps everything but the first line (not intended).
I don't know why this was introduced in the first place.
There was a problem hiding this comment.
Don't see an obvious reason to have it.
There was a problem hiding this comment.
When the comment has a line break, it keeps everything but the first line (not intended).
Yeah this seems extra bad. Probably breaks the JS syntax wise?
I don't know why this was introduced in the first place.
I think Flow uses the export lines and they would have worked except for the cylic dependencies. So, @unhammer just effectively commented them out before passing to flow in anticipation of eventually fixing the cyclic dependencies and making it so flow would actually use the export/imports. So, it should be removable now.
|
When running |
I believe using |
|
The |
|
Oops, clicked the wrong button. |
We need an extra file |
flow-typed/apertium.js
Outdated
| @@ -0,0 +1,35 @@ | |||
| declare module 'util' { | |||
There was a problem hiding this comment.
Parsing error: Unexpected token module
There was a problem hiding this comment.
Hound's configuration allows for excluding files, I believe that would work.
There was a problem hiding this comment.
Is this triggered by jshint or eslint ? Shall I exclude this from both?
There was a problem hiding this comment.
jshint isn't being used for this project...
|
Lol, I don't know why the build is passing. It certainly shouldn't be (see below). This is a lot of modifications so I don't think I expect you to fix them all. As long as you get flow actually running, it's fine. I don't think |
But the errors are gone as of adding this file. |
It has to be a false negative. A lot of these errors are valid ones. I also don't know how |
|
|
See my previous comment. Adding that file should not get rid of all those errors. |
|
This was just due to Flow not being able to resolve those references (across modules). |
|
No. The exports and imports are not correct. e.g. |
You mean, in addition to |
|
Yes. In fact, there are over 20 other similar errors. I don't think the way you changed the Makefile is actually running flow anymore. |
|
These changes cut down the # of errors to 41 but there are still many more: diff --git a/.flowconfig b/.flowconfig
index 28eb815..d5f5265 100644
--- a/.flowconfig
+++ b/.flowconfig
@@ -1,5 +1,5 @@
[ignore]
-build/
+.*/build/.*
[include]
diff --git a/assets/js/translator.js b/assets/js/translator.js
index d137f30..9872b7d 100644
--- a/assets/js/translator.js
+++ b/assets/js/translator.js
@@ -24,7 +24,7 @@ var PUNCTUATION_KEY_CODES = [46, 33, 58, 63, 47, 45, 190, 171, 49]; // eslint-di
/* global config, modeEnabled, synchronizeTextareaHeights, persistChoices, getLangByCode, sendEvent, onlyUnique, restoreChoices
getDynamicLocalization, locale, ajaxSend, ajaxComplete, localizeInterface, filterLangList, cache, readCache, iso639Codes,
callApy, apyRequestTimeout, isURL */
-/* global SPACE_KEY_CODE, ENTER_KEY_CODE, HTTP_OK_CODE, XHR_LOADING, XHR_DONE, HTTP_OK_CODE, HTTP_BAD_REQUEST_CODE */
+/* global SPACE_KEY_CODE, ENTER_KEY_CODE, HTTP_OK_CODE, XHR_LOADING, XHR_DONE, HTTP_BAD_REQUEST_CODE */
if(modeEnabled('translation')) {
$(document).ready(function () {
@@ -1074,10 +1074,11 @@ function autoSelectDstLang() {
}
}
-/*:: import {synchronizeTextareaHeights, modeEnabled, ajaxSend, ajaxComplete, filterLangList, onlyUnique, getLangByCode,
- callApy, apyRequestTimeout} from "./util.js" */
+/*:: export {populateTranslationList, showTranslateWebpageInterface} */
+/*:: import {synchronizeTextareaHeights, modeEnabled, ajaxSend, ajaxComplete, filterLangList, onlyUnique, sendEvent, XHR_DONE,
+ callApy, apyRequestTimeout, ENTER_KEY_CODE, SPACE_KEY_CODE, HTTP_OK_CODE, HTTP_BAD_REQUEST_CODE, XHR_LOADING} from "./util.js" */
/*:: import {persistChoices, restoreChoices} from "./persistence.js" */
-/*:: import localizeInterface from "./localization.js" */
-/*:: import {readCache,cache} from "./cache.js" */
+/*:: import {localizeInterface, getLangByCode, getDynamicLocalization} from "./localization.js" */
+/*:: import {readCache, cache} from "./persistence.js" */
/*:: import {config} from "./config.js" */
/*:: import {isURL} from "./util.js" */
diff --git a/assets/js/util.js b/assets/js/util.js
index af13560..b914d87 100644
--- a/assets/js/util.js
+++ b/assets/js/util.js
@@ -304,7 +304,7 @@ function getURLParam(name) {
/* eslint-disable */
// From: http://stackoverflow.com/a/19696443/1266600 (source: AOSP)
-function isURL(text) {
+function isURL(text/*: string*/) {
var re = /^((?:(http|https):\/\/(?:(?:[a-zA-Z0-9\$\-\_\.\+\!\*\'\(\)\,\;\?\&\=]|(?:\%[a-fA-F0-9]{2})){1,64}(?:\:(?:[a-zA-Z0-9\$\-\_\.\+\!\*\'\(\)\,\;\?\&\=]|(?:\%[a-fA-F0-9]{2})){1,25})?\@)?)?((?:(?:[a-zA-Z0-9][a-zA-Z0-9\-]{0,64}\.)+(?:(?:aero|arpa|asia|a[cdefgilmnoqrstuwxz])|(?:biz|b[abdefghijmnorstvwyz])|(?:cat|com|coop|c[acdfghiklmnoruvxyz])|d[ejkmoz]|(?:edu|e[cegrstu])|f[ijkmor]|(?:gov|g[abdefghilmnpqrstuwy])|h[kmnrtu]|(?:info|int|i[delmnoqrst])|(?:jobs|j[emop])|k[eghimnrwyz]|l[abcikrstuvy]|(?:mil|mobi|museum|m[acdghklmnopqrstuvwxyz])|(?:name|net|n[acefgilopruz])|(?:org|om)|(?:pro|p[aefghklmnrstwy])|qa|r[eouw]|s[abcdeghijklmnortuvyz]|(?:tel|travel|t[cdfghjklmnoprtvwz])|u[agkmsyz]|v[aceginu]|w[fs]|y[etu]|z[amw]))|(?:(?:25[0-5]|2[0-4][0-9]|[0-1][0-9]{2}|[1-9][0-9]|[1-9])\.(?:25[0-5]|2[0-4][0-9]|[0-1][0-9]{2}|[1-9][0-9]|[1-9]|0)\.(?:25[0-5]|2[0-4][0-9]|[0-1][0-9]{2}|[1-9][0-9]|[1-9]|0)\.(?:25[0-5]|2[0-4][0-9]|[0-1][0-9]{2}|[1-9][0-9]|[0-9])))(?:\:\d{1,5})?)(\/(?:(?:[a-zA-Z0-9\;\/\?\:\@\&\=\#\~\-\.\+\!\*\'\(\)\,\_])|(?:\%[a-fA-F0-9]{2}))*)?(?:\s*$)/i;
return text.search(re) === 0;
}
@@ -408,7 +408,7 @@ function displayInstallationNotification() {
}
/*:: export {synchronizeTextareaHeights, modeEnabled, ajaxSend, ajaxComplete, filterLangList, onlyUnique, callApy,
- SPACE_KEY_CODE, ENTER_KEY_CODE, HTTP_OK_CODE, HTTP_BAD_REQUEST_CODE, XHR_LOADING, XHR_DONE, apyRequestTimeout} */
+ SPACE_KEY_CODE, ENTER_KEY_CODE, HTTP_OK_CODE, HTTP_BAD_REQUEST_CODE, XHR_LOADING, XHR_DONE, apyRequestTimeout, isURL, sendEvent} */
/*:: import {config} from "./config.js" */
/*:: import {persistChoices} from "./persistence.js" */
/*:: import {iso639Codes, iso639CodesInverse} from "./localization.js" */ |
|
I can fix them, no problem. |
|
Imports/Exports were no problem. |
Don't worry about it. I have a plan to handle those (i.e. fix some myself and make a task/issue for the rest). This task will be complete if you clean up the extra files and stuff in this PR. |
.circleci/config.yml
Outdated
|
|
||
| # Build project | ||
| - run: cp config.conf.example config.conf | ||
| - run: flow check |
There was a problem hiding this comment.
Suggest moving this up to the block above now.
.eslintignore
Outdated
| @@ -0,0 +1 @@ | |||
| flow-typed/*.js | |||
There was a problem hiding this comment.
Should be safe to kill this folder and files in it now.
Makefile
Outdated
| # grab the bin from https://github.com/facebook/flow/releases | ||
| flow: build/js/all.js | ||
| grep -Ev '/\*:: *(ex|im)port ' $< | flow check-contents | ||
| flow: $(JSFILES) |
There was a problem hiding this comment.
I think this make command can be removed now.
assets/js/translator.js
Outdated
| getDynamicLocalization, locale, ajaxSend, ajaxComplete, localizeInterface, filterLangList, cache, readCache, iso639Codes, | ||
| callApy, apyRequestTimeout, isURL */ | ||
| /* global SPACE_KEY_CODE, ENTER_KEY_CODE, HTTP_OK_CODE, XHR_LOADING, XHR_DONE, HTTP_OK_CODE, HTTP_BAD_REQUEST_CODE */ | ||
| callApy, apyRequestTimeout, isURL, iso639Codes, iso639CodesInverse */ |
There was a problem hiding this comment.
'iso639CodesInverse' is defined but never used no-unused-vars
* [WIP] Disentangle JS modules (#225) * Disentangle JS modules * Fix imports * Use flow on individual files * Declare modules to Flow * Run Flow directly * Cleanup * Fix some import/export issues * Make the build work * Tweak flow decl * Move around the config decl to make build work
|
@Androbin if you'd like to take fixing up the types as a task, you can do so under the auspices of #228. My commit to master also contains some help with syntax (e.g. for generics) and model for how the comments should be formatted. For purposes of task completion, I deem sufficient changes to be the lower of 20% overall improvement or 350 LOC modified (i.e. you can do either). |
* Add file download workaround for Safari (fixes #97) * Changed Download_Browser_Warning * Added workaround for Safari Fixed translator.js Fixed wierd bugs that I have no idea how I had made. * Moved .fadeIn out of conditional block * Added comments to increase code readability * Allow right-click to download even if workaraound is used * Revert string changes * Fix multiple download in Safari >10 Fix es-lint Fix es-lint * Removed Download_Browser_Warning * Change way we check for Safari version Instead of looking at specific version (10, 11) look for one digit version (9, 8 and earlier) * Remove unused global varaibles * Extended comment about Safari workaround * Run cleanup * Fixes #201 (Chrome audit improvements) * Improved Chrome audit performance * Update Makefile * Fixed formatting errors * Fixed more formatting errors * Revised "start_url" * Removed sw.js * Removed script for registering service worker * Revised makefile, manifest.json, and index.html * Improved Chrome audit performance further * Improved audit performance further * Fixed errors * fixed houndci error * Fixed final errors * Added whitespace * Fix <html> localisation * Disentangle JS modules and fix Flow (fixes #80) * [WIP] Disentangle JS modules (#225) * Disentangle JS modules * Fix imports * Use flow on individual files * Declare modules to Flow * Run Flow directly * Cleanup * Fix some import/export issues * Make the build work * Tweak flow decl * Move around the config decl to make build work * Delete extra line * Fix make debug (fixes #220) * Improvements to Flow coverage * Better flow coverage * Doesn't look like the stubs actually belong to jQuery 3 so I'm reverting these changes * Remove unnecessary var * This name makes a lot more sense * Remove unnecessary type annotations (clutter code & are obvious e.g. string literals) * More unnecessary type annotations * Remove more unnecessary annotations * Prefer literal union type * Add a type and remove one * More fixes * Fix persist choices * Remove more redundant annotations * Remove more unnecessary annotations * Switch to Array<...> shortform * Undo one of my 'fixes' * Final flow fix * Convert Object to {} shorthand * Remove a couple more unnecessary annotations * Some final cleanup * Implement requested changes * Add/Remove/Fix types * Adapt config types * Un-Flow strict.js * Allow subtitle to be null * Final tweaks * Simplify progress bar update * Split CSS into multiple files and improve organization (fixes #212) * Split CSS into multiple files * Better organization * Minor tweaks * Bring all the coverage over 90% * Fix buggy UI with detect-language (fixes #236) * Refactor repeated code into a function * Fix the locale selector I probably broke * Show translation not available less often * Remove a warn that gets triggered all the time * More generic port configuration with Docker * Less awkward input label placement * Slightly more sane language sorting with variants * Significantly more sane translation language column computation * Fix typo * Store docker pair data in persistent volume and add update all script * Include language modules download in deploy script (fixes #244) * Minify JS and CSS (towards #130) * Minify all.js and all.css * Make minification fail gracefully * Update README section on compression * Use command line interfaces * Update Dockerfile * Update README.md * Update README.md * Fix dockerfiles * Fix debug HTML * Fetch monolingual modules before pairs in deploy-all script * Merge handleTranslateSuccessResponse * Update imports/exports * Add JQuery modal/tooltip stubs * Add config.SUGGESTIONS stubs * Fix remaining Flow issues
3 participants
Fixing #80