basis: Add tests, improve options handling, clean up demo #596
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xintongxia
reviewed
Dec 31, 2019
| // Load from local files, not from CDN scripts in Node.js | ||
| // TODO - needs to locate the modules directory when installed! | ||
| if (!isBrowser) { | ||
| return `modules/${moduleName}/dist/libs/${library}`; |
There was a problem hiding this comment.
Since this is inside util module which will be shared by other modules, better to have stable way to resolve the module directory. Add an issue to track this?
There was a problem hiding this comment.
Yes good point, the dynamic module loading was a bit rushed in order to get ready for v2.0.
I will put together a separate, smaller PR with test cases for the library-util functions and we can iterate on design etc there.
| if (options.CDN) { | ||
| assert(options.CDN.startsWith('http')); | ||
| return `${options.CDN}/${moduleName}@${VERSION}/dist/libs/${library}`; | ||
| } | ||
|
|
||
| // Note - Worker loading requires paths relative to worker script "location"? | ||
| // TODO - loading inside workers requires paths relative to worker script location... | ||
| if (isWorker) { | ||
| return `../src/libs/${library}`; |
There was a problem hiding this comment.
same here. To me, it is kind of fragile to resolve a path like this way.
| @@ -11,6 +11,9 @@ import path from 'path'; | |||
| // This indirect function is provided because webpack will try to bundle `module.require`. | |||
| // this file is not visible to webpack (it is excluded in the package.json "browser" field). | |||
| export function requireFromFile(filename) { | |||
| if (filename.startsWith('http')) { | |||
ibgreen
force-pushed
the
ib/basis-tests
branch
from
8549c2c
to
294e0fc
Compare
xintongxia
pushed a commit
that referenced
this pull request
Jan 4, 2020
xintongxia
pushed a commit
that referenced
this pull request
Jan 11, 2020
…)" This reverts commit b2fa7bd.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A pass on the experimental
BasisLoader