Add UPWARD specification, guide, tests, reference implementation #248
Conversation
zetlen
force-pushed
the
zetlen/upward
branch
2 times, most recently
from
5d3af8b
to
fadb5f7
Compare
packages/upward-js/bin/server
Outdated
| if (mapping.hasOwnProperty(lowered)) { | ||
| return mapping[lowered]; | ||
| } | ||
| if (value.match(/^\d*\.?\d+$/)) { |
There was a problem hiding this comment.
Consider adding a comment either explaining the regex or giving an example.
packages/upward-js/lib/Context.js
Outdated
| 'latin-1', | ||
| 'base64', | ||
| 'hex', | ||
| ...Array.from({ length: 600 }, (_, i) => (i + 100).toString()) |
packages/upward-js/lib/Context.js
Outdated
| } | ||
|
|
||
| get(path) { | ||
| return constants.get(prop) || dotLookup(this._data, path); |
There was a problem hiding this comment.
Is dotLookup a global or something? I don't see it defined or imported. Perhaps you meant this.depend, which is defined below but appears to be unused?
| return value => | ||
| allowed.includes(value) | ||
| ? undefined | ||
| : `must be one of: ${allowed.join()}`; |
There was a problem hiding this comment.
In the passing case, why does this return undefined, rather than '' like all the rest of these tests?
| errors.push({ | ||
| name, | ||
| errors: itemErrors | ||
| }); |
There was a problem hiding this comment.
It doesn't look like errors is defined at this point, so errors.push looks wrong to me here. Did you mean report.errors.push()?
zetlen
force-pushed
the
zetlen/upward
branch
from
ad2be63
to
6b92359
Compare
|
Noting that I have to back-merge my integration branch into this. |
zetlen
force-pushed
the
zetlen/upward
branch
from
66ea456
to
5fd2d40
Compare
|
Closes #28. |
zetlen
force-pushed
the
zetlen/upward
branch
from
f3647c2
to
94383b6
Compare
| return x; | ||
| }, rejectPreload); | ||
| } catch (e) { | ||
| rejectPreload(); |
There was a problem hiding this comment.
It looks like rejectPreload accepts a single argument, e. It's not being called with any arguments here, though. Did you mean to pass e to it?
| mod => mod.resource.includes('/node_modules/') | ||
| : // Top-level modules injected by a downstream "issuer" are not | ||
| // entry points in production. | ||
| mod => !mod.issuer; |
There was a problem hiding this comment.
Comments helping make this file understandable. 👍
| // apply their changes synchronously. Use a proxy function to add a | ||
| // middleware which lazy loads UPWARD when it's time. | ||
| let upwardMiddleware; | ||
| let upwardMiddlewarePromise = new Promise((resolve, reject) => {}); |
There was a problem hiding this comment.
This variable, upwardMiddlewarePromise, appears to be unused. Is this dead code? How is this supposed to work?
| : res.json() | ||
| ); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Clean and understandable. 👍
Tests would be good.
| } | ||
| } | ||
|
|
||
| module.exports = Context; |
There was a problem hiding this comment.
Can you explain what this file is doing?
| @@ -0,0 +1,101 @@ | |||
| const debug = require('debug')('upward-js:ContextPath'); | |||
| const illegalPathChars = /(^[\.\[\]])|[^\.\w\$\/]/; | |||
There was a problem hiding this comment.
Consider adding a comment either explaining the regex or giving an example.
|
|
||
| const readFile = promisify(fsReadFile); | ||
| class IOAdapter { | ||
| static default(upwardPath) { |
| debug( | ||
| `returning middleware from visitor.downward() instead of object` | ||
| ); | ||
| return passedMiddleware; |
There was a problem hiding this comment.
So if any one of the requests returns a middleware, this error is thrown and caught, and only the middleware is returned. All the other requests are thrown out?
| } | ||
| getResolverFor(defined, propertyName) { | ||
| let Resolver; | ||
| for (Resolver of ResolverList) { |
There was a problem hiding this comment.
The scoping of Resolver is misleading here.
Resolveris declared as a mutable variable prior to thefor...ofblock- On each loop iteration,
Resolveris reassigned andResolver.recognize()is always called - On each loop iteration, there may be an early return
The only time this arrangement is useful is if the code following the for...of block is interested in the value of Resolver after the loop has completed. Otherwise, we would expect to see for (const Resolver of ResolverList) {}.
However, the code following the for...of block ("subsequent code") is not interested in the value of Resolver. In fact, the subsequent code always reassigns Resolver before ever reading its value. This is counterintuitive because Resolver appears to have been specifically declared such that it is meaningful, yet it isn't.
My recommendation:
getResolverFor(defined, propertyName) {
for (const Resolver of ResolverList) {
// ...
}
const definedResolver = defined.resolver
const Resolver = definedResolver
? ResolversByType[definedResolver]
: ResolverList.find(({ telltale }) => defined.hasOwnProperty(telltale))
if (Resolver) {
return new Resolver(this)
} else {
this.getResolverFailure = definedResolver
? 'Unrecognized resolver type...'
: 'Unrecognized configuration...'
}
}
zetlen
changed the title
Generated by 🚫 dangerJS |
This PR is a:
[x] New feature
[ ] Enhancement/Optimization
[ ] Refactor
[ ] Bugfix
[ ] Test for existing code
[x] Documentation
Summary
When this pull request is merged, it will add the UPWARD specification, guide, test suite, and reference implementation in JavaScript. See the README in this pull request for details.