refactor(env): Extract Prototypes into separate files #1100

ExE-Boss · 2019-03-19T22:15:36Z

This extracts all the *Prototype objects into separate files in src/api/*, which will make testing easier.

To do:

util
mdnPrototype
stringPrototype (I’ll also apply my fixes from test(macros/DekiScript): Add tests for DekiScript:String #1013)
wikiPrototype
webPrototype
pagePrototype

Simple re‑exports:

I’m considering leaving these ones in environment.js, as they just re‑export functions:

~~globalsPrototype~~ (reverted in 279561a)
kumaPrototype
uriPrototype

I’m also adding JSDoc comments from #1077.

review?(@davidflanagan, @escattone)

davidflanagan

Thank you ExE-Boss! This is something that I realized I wanted to do after my big patch had been reviewed. But I never made the time to get back to it.

I have not reviewed the comments (and the types in particular) that you've added. And as we've discussed before, since this project is not configured to use a type checker, we can't guarantee that the functions will always be used in a way that is consistent with the types you've declared.

One of the reasons that I wanted to make this change is to get all the untested API code out of environment.js and move it all to the api/ subdirectory. So I'd be really interested to see how this patch affects the output of npm run test-coverage

Github does not make it easy to review a change like this that splits one file up into multiple pieces: it ends up looking like lots of new code to review. Can you confirm that all you did was split the *Prototype objects into separate modules and add comments? Were there any code changes involved?

Overall this looks great. I've made a couple of minor suggestions, and I'll be happy to merge this after you make those changes. (That is assuming you haven't made code changes to any of the API functions)

src/environment.js

ExE-Boss · 2019-03-21T00:09:39Z

The issue with globalsAPI is that it exposes the unmodified require(…) function.

By moving it to the api directory, this would change the behaviour of relative require(…).

I’ve modified the code so that relative require(…) in macros is always resolved against the config.macrosDirectory, rather than the src or src/api directory.

ExE-Boss · 2019-03-21T00:16:38Z

src/api/uri.js

+ * @return {string}
+ */
+function encode(str) {
+    return encodeURI(str);


This should probably be changed to call encodeURIComponent(…) instead.

I don't think it would be safe to make that change. It probably should have been written that way to begin with, but any change now risks changing what appears on developer.mozilla.org and I don't want to risk that since we don't have a way to test that API changes here don't affect the output on any of the thousands of MDN document, I'm very very reluctant to change anything.

ExE-Boss

In wiki.tree(…): chunkify(…), alpanumForward(…) and alphanumBackward(…) are getting type errors from tsc:

ExE-Boss · 2019-03-21T00:25:02Z

src/api/wiki.js

+
+        while ((i = (j = t.charAt(x++)).charCodeAt(0))) {
+            var m = i == 46 || (i >= 48 && i <= 57);
+            if (m !== n) {


error TS2367: This condition will always return 'true' since the types 'boolean' and 'number' have no overlap.

I wonder if this code was a port of a C algorithm, and n=0 above was intended to be n=false? Maybe the !== should have been a !=

But again, unless we can point to a specific content bug that is being caused by this, I don't want to go changing this legacy code for fear of causing unexpected changes to the output.

ExE-Boss · 2019-03-21T00:25:09Z

src/api/wiki.js

+            var m = i == 46 || (i >= 48 && i <= 57);
+            if (m !== n) {
+                tz[++y] = '';
+                n = m;


error TS2322: Type 'boolean' is not assignable to type 'number'.

ExE-Boss · 2019-03-21T00:25:58Z

src/api/wiki.js

+            if (aa[x] !== bb[x]) {
+                var c = Number(aa[x]),
+                    d = Number(bb[x]);
+                if (c == aa[x] && d == bb[x]) {


error TS2367: This condition will always return 'false' since the types 'number' and 'string' have no overlap.

That's just not true. (1 == "1") evaluates to true in JavaScript

It looks like this code is so old and untyped that it is not possible to apply type analysis to it without a complete rewrite. And rewrites are off the table.

ExE-Boss · 2019-03-21T00:26:08Z

src/api/wiki.js

+            if (aa[x] !== bb[x]) {
+                var c = Number(aa[x]),
+                    d = Number(bb[x]);
+                if (c == aa[x] && d == bb[x]) {


error TS2367: This condition will always return 'false' since the types 'number' and 'string' have no overlap.

davidflanagan

If you don't mind, please put globalsAPI back in environment.js to avoid the complication of needing util.createRequire().

Also, before I can merge this I still need you to tell me whether this is purely a code move plus comments or whether you changed any of the code when you moved it into separate files. Changing any of the implementations of these API functions feels very risky to me, and I don't want to risk that.

src/api/globals.js

davidflanagan · 2019-03-22T17:08:37Z

src/api/uri.js

+ * @return {string}
+ */
+function encode(str) {
+    return encodeURI(str);


I don't think it would be safe to make that change. It probably should have been written that way to begin with, but any change now risks changing what appears on developer.mozilla.org and I don't want to risk that since we don't have a way to test that API changes here don't affect the output on any of the thousands of MDN document, I'm very very reluctant to change anything.

davidflanagan · 2019-03-22T17:13:04Z

src/api/wiki.js

+
+        while ((i = (j = t.charAt(x++)).charCodeAt(0))) {
+            var m = i == 46 || (i >= 48 && i <= 57);
+            if (m !== n) {


I wonder if this code was a port of a C algorithm, and n=0 above was intended to be n=false? Maybe the !== should have been a !=

But again, unless we can point to a specific content bug that is being caused by this, I don't want to go changing this legacy code for fear of causing unexpected changes to the output.

davidflanagan · 2019-03-22T17:14:47Z

src/api/wiki.js

+            if (aa[x] !== bb[x]) {
+                var c = Number(aa[x]),
+                    d = Number(bb[x]);
+                if (c == aa[x] && d == bb[x]) {


That's just not true. (1 == "1") evaluates to true in JavaScript

davidflanagan · 2019-03-22T17:15:34Z

src/api/wiki.js

+            if (aa[x] !== bb[x]) {
+                var c = Number(aa[x]),
+                    d = Number(bb[x]);
+                if (c == aa[x] && d == bb[x]) {


It looks like this code is so old and untyped that it is not possible to apply type analysis to it without a complete rewrite. And rewrites are off the table.

ExE-Boss · 2019-03-22T20:57:05Z

I haven’t actually changed any code, the only thing I’ve done is add TODO and FIXME comments.

I’ve kept the createRequire(…) function in util.js with a TODO comment to use it eventually once tests are written for it.

davidflanagan · 2019-03-25T21:59:51Z

I just saw that you pushed another change... Let me know when you're done and I'll take a final look and merge. I really would prefer that you put the '.js' extensions back on the require() calls, though.

ExE-Boss · 2019-03-25T22:30:50Z

I’d say I’m done now.

davidflanagan · 2019-03-25T22:49:37Z

I just noticed what you said above about keeping createRequire() in utils with a TODO comment. Please don't do that. I'd like to put KumaScript into maintenance mode and I don't think it will be worth while to make that kind of change in the future.

And again, please add '.js' extensions to your require() calls, or try to convince me why we shouldn't be using them. My argument for using extensions is that without them the code is less clear and it is not clear what would happen if you were to drop a wiki.json or wiki.ts file into the api/ directory. I'd prefer to avoid code that depends on the precise behavior of the module loader and the contents of directories.

ExE-Boss · 2019-03-25T23:13:39Z

Well, I guess I can always just change them to .ts as part of the conversion to TypeScript.

davidflanagan

A couple more changes, please.

Also: what conversion to typescript are you talking about? Have you discussed this with anyone? Maybe I'm just out of the loop, but I can tell you now that I would not be willing to review any such patch.

It is unclear what the future of KumaScript is. MDN documentation will probably continue to rely on it for some time, but there are not any plans to add features to KumaScript or to increase our dependency on it by adding more macros than we delete. If you have your own reasons for wanting to work on and evolve KumaScript, you might need to create your own fork.

src/api/util.js

davidflanagan · 2019-03-27T22:27:39Z

I was hoping to merge this today, but in doing a final check, I found some more changes I'd like you to make. A couple are just nits, but one is serious.

npm run format makes a couple of formatting changes to environment.js. Please fix that. (It also changes a test file that you have not touched, but feel free to fix that one as well.)
In Makefile and package.json there are targets for test-coverage, but they only scan src/*, and now that src has a subdirectory they need to be updated to src/**, I think.
In src/api/strings.js, you change the casing of the functions. For example, you changed ToUpperFirst() to toUpperFirst(). I agree that the original names are horrible and ugly, but since we have no good way to test changes here, we have to be completely paranoid about compatibility. The prepareProto() function messes with case, but not equivalently. ToUpperFirst was available as ToUpperFirst() and as toupperfirst(), but it was never available as toUpperFirst(), so by adding that you've introduced a new function into the API. That is not likely to break things, but it could, and I don't want to risk it. So please revert these functions to their original uppercase form.
When I was imagining moving these chunks of code into src/api, what I envisioned was files that looked like this:

// a couple of requires, as needed

module.exports = /* an object literal copied verbatim from environment.js */;

Environment.js already has the APIs defined as object literals with methods inside them, and I was imagining that you could use those object literals exactly as they are with the nice method syntax and without having to add the function keywords. Doing that would make it clearer that nothing had changed and that the refactor was completely safe. As it is, the formatting and indentation changes enough that it is harder to see what has changed.

I would prefer it if these api files were in that module.exports = {big object with methods}; form. But if you feel like that would be too much work to do that conversion at this point, I'm willing to accept them as-is.

ExE-Boss · 2019-03-27T23:56:53Z

Turns out, the issue was trailing whitespace, which somehow didn’t get removed by my IDE.

And also a trailing comma, which were used in many places before #1035.

Also, I did a search, and nothing depends on the existence of lowerCamelCase versions of the stringAPI namespace methods, except for when I write my code, and then notice that it doesn’t work, because prepareProto(…) assumes that the default method name is in the lowerCamelCase variant, and so it doesn’t generate one.

davidflanagan · 2019-03-28T00:23:36Z

The thing about the string API is that it is a horrible API that macro writers would be better off not using. Giving the functions better names will only make the API more attractive to use, which is the opposite of what we want.

The prepareProto() function doesn't make any assumptions about the method name beginning with a lower case letter: it simply matches the legacy behavior of the old KumaScript. Since we're not actively working on KumaScript anymore, I really don't want any risk of subtle changes that could cause future maintenance issues.

Changing the case of those function names converts this PR from a pure refactor with no behavior changes into something else. We'll have to get input from other team members if you want to proceed with the lower-case variants.

davidflanagan

ExE-Boss,

I think I have been quite clear that I'm paranoid about changes to this code and that I am unwilling to accept any changes other than breaking environment.js into new files and adding comments. Yet in this most recent patch you included two code changes. That makes me concerned that you may have made other small changes like these when you moved the code out of environment.js. Because of the way the files were reformatted, however, I have no way of determining which lines, if any were changed.

I believe that both of these change in this latest commit are equivalent to the old code, and may well be faster as well. I understand that you are a skilled programmer and can get stuff like this right. However, that doesn't reduce my burden of reviewing your changes. There are lots of things that could be improved in this code, but because it is old legacy code that no one on the team is particularly familiar with and becasue of the lack of comprehensive tests, every little improvement is risky, and every change requires me to spend time convincing myself that it is safe.

This is not a patch that is on anyone's priority list. I like the idea of it, but my manager would not be happy to know how much time I've spent this week on reviewing it.

This may sound unreasonable to you, but in order for me to approve this patch at this point, I need to have the code copied verbatim (without whitespace changes, even) from environment.js into api/.js. I want to be able to concatenate the files in api/.js (in whatever the correct order is) and then diff that concatenated file against the old version of environment.js and see only changes to comments and a few changes from lines like const mdnPrototype = { to module.exports = {

I really would like to have the APIs broken out like this, because it is more elegant and because it improves our test coverage percentage for environment.js. But at this point I'll only accept the patch if I can mechanically verify that the code is unchanged. I will understand if you prefer to just abandon the PR at this point.

src/api/mdn.js

ExE-Boss · 2019-03-28T18:31:08Z

Ok, I’ll make a new PR where I don't make any changes, and then rebase this on top of it.

ExE-Boss · 2019-03-28T18:53:15Z

I’ve now opened #1103 to do that.

ExE-Boss added 2 commits March 19, 2019 23:09

refactor(env): Extract util into separate file

98dd180

refactor(env): Extract mdn into separate file

2569a35

ExE-Boss force-pushed the refactor/separate-environment-prototypes branch from dc5ddf3 to 7699daa Compare March 19, 2019 23:50

refactor(env): Extract string into separate file

7699daa

ExE-Boss force-pushed the refactor/separate-environment-prototypes branch from 860d686 to c73f107 Compare March 20, 2019 00:24

ExE-Boss added 3 commits March 20, 2019 01:25

refactor(env): Extract wiki into separate file

c73f107

refactor(env): Extract web into separate file

42b0c15

refactor(env): Extract page into separate file

54c62b5

ExE-Boss closed this Mar 20, 2019

ExE-Boss reopened this Mar 20, 2019

ExE-Boss force-pushed the refactor/separate-environment-prototypes branch 2 times, most recently from a96397f to 89c3b3c Compare March 20, 2019 00:38

ExE-Boss closed this Mar 20, 2019

ExE-Boss reopened this Mar 20, 2019

ExE-Boss force-pushed the refactor/separate-environment-prototypes branch 2 times, most recently from 663d1a9 to 828dd58 Compare March 20, 2019 12:36

docs(env): Re‑organise and update JSDoc

828dd58

davidflanagan suggested changes Mar 20, 2019

View reviewed changes

src/environment.js Outdated Show resolved Hide resolved

src/environment.js Outdated Show resolved Hide resolved

refactor(env): Extract remaining API objects

fb9d4ff

ExE-Boss commented Mar 21, 2019

View reviewed changes

davidflanagan suggested changes Mar 22, 2019

View reviewed changes

refactor(env): Keep globalsAPI in environment.js for now

279561a

ExE-Boss mentioned this pull request Mar 22, 2019

refactor: Use extensionless require #1099

Closed

ExE-Boss force-pushed the refactor/separate-environment-prototypes branch from 7ed4d3c to 461c29f Compare March 25, 2019 21:44

docs(env.util): Use ComputeValue as parameter type for promisify(…)

461c29f

refactor: Use require(…) with file extensions

5359200

davidflanagan suggested changes Mar 26, 2019

View reviewed changes

src/api/util.js Show resolved Hide resolved

src/api/util.js Outdated Show resolved Hide resolved

refactor(env): Remove util.createRequire(…)

b661d20

refactor(env): pnpm run format

1e4c759

ExE-Boss force-pushed the refactor/separate-environment-prototypes branch from 7c75549 to 26e416d Compare March 28, 2019 14:17

Revert stringAPI case changes

26e416d

davidflanagan suggested changes Mar 28, 2019

View reviewed changes

src/api/mdn.js Outdated Show resolved Hide resolved

src/api/mdn.js Outdated Show resolved Hide resolved

Fix type‑unsafe code

601fbb2

This was referenced Mar 28, 2019

refactor(env): Extract Prototypes into separate files #1102

Closed

refactor(env): Extract Prototypes into separate files #1103

Merged

ExE-Boss closed this Mar 28, 2019

ExE-Boss mentioned this pull request Mar 30, 2019

docs(env): Add JSDoc to Environment and API prototypes #1107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(env): Extract Prototypes into separate files #1100

refactor(env): Extract Prototypes into separate files #1100

ExE-Boss commented Mar 19, 2019 •

edited

davidflanagan left a comment

ExE-Boss commented Mar 21, 2019 •

edited

ExE-Boss Mar 21, 2019

davidflanagan Mar 22, 2019

ExE-Boss left a comment

ExE-Boss Mar 21, 2019

davidflanagan Mar 22, 2019

ExE-Boss Mar 21, 2019

ExE-Boss Mar 21, 2019

davidflanagan Mar 22, 2019

davidflanagan Mar 22, 2019

ExE-Boss Mar 21, 2019

davidflanagan left a comment

davidflanagan Mar 22, 2019

davidflanagan Mar 22, 2019

davidflanagan Mar 22, 2019

davidflanagan Mar 22, 2019

ExE-Boss commented Mar 22, 2019 •

edited

davidflanagan commented Mar 25, 2019

ExE-Boss commented Mar 25, 2019 •

edited

davidflanagan commented Mar 25, 2019

ExE-Boss commented Mar 25, 2019

davidflanagan left a comment

davidflanagan commented Mar 27, 2019

ExE-Boss commented Mar 27, 2019

davidflanagan commented Mar 28, 2019

davidflanagan left a comment

ExE-Boss commented Mar 28, 2019

ExE-Boss commented Mar 28, 2019

refactor(env): Extract Prototypes into separate files #1100

refactor(env): Extract Prototypes into separate files #1100

Conversation

ExE-Boss commented Mar 19, 2019 • edited

To do:

Simple re‑exports:

davidflanagan left a comment

Choose a reason for hiding this comment

ExE-Boss commented Mar 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExE-Boss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidflanagan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExE-Boss commented Mar 22, 2019 • edited

davidflanagan commented Mar 25, 2019

ExE-Boss commented Mar 25, 2019 • edited

davidflanagan commented Mar 25, 2019

ExE-Boss commented Mar 25, 2019

davidflanagan left a comment

Choose a reason for hiding this comment

davidflanagan commented Mar 27, 2019

ExE-Boss commented Mar 27, 2019

davidflanagan commented Mar 28, 2019

davidflanagan left a comment

Choose a reason for hiding this comment

ExE-Boss commented Mar 28, 2019

ExE-Boss commented Mar 28, 2019

ExE-Boss commented Mar 19, 2019 •

edited

ExE-Boss commented Mar 21, 2019 •

edited

ExE-Boss commented Mar 22, 2019 •

edited

ExE-Boss commented Mar 25, 2019 •

edited