-
-
Notifications
You must be signed in to change notification settings - Fork 678
[Implement] Universal Resolver Package #796
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
lib/resolver/index.js
Outdated
| const fileRelativePath = this.file; | ||
| const packageJsonPath = path.join(moduleFolder, "package.json"); | ||
| const pkg = readFileSync(packageJsonPath); | ||
| let ascFolder = "assembly"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess better move "assembly" out to top level const and call something like const ASC_DEFAULT_FOLDER = "assembly", and here use let ascFolder = ASC_DEFAULT_FOLDER
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't satisfied with any of the implementation. This will likely change. Considder my pull request a draft.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also appears that @willemneal is porting this whole algorithm to TypeScript. Should be able to generate the Types and portable es5 js.
jtenner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/resolver/src/index.ts
Outdated
| Package = 2, | ||
| } | ||
|
|
||
| export function resolve(fromSourcePath, toSourcePath, args, sourcePathType = SourceType.Local) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing about the lib folders in the args object is that it should be an array of absolute paths. Perhaps we should name the parameters this:
export function resolve(absoluteFromSource: string, relativeToSource: string, absoluteLibFolders: string[] = [], pathFolders: string[] = [], fromSourceType: SourceType): Entry[];Also, what should we name the Entry class? It represents a potential module resolution point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe Address? Also how about args as an interfaces { lib: string[]; path: string[]} so we can just pass that in from the compiler as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do lib folders work if they are relative to cwd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they have to be relative to baseDir. So we could just turn them into absolute paths, or pass a baseDir.
lib/resolver/src/index.ts
Outdated
| : string = null; | ||
| url: url.UrlWithStringQuery = null; | ||
|
|
||
| constructor(public file: string = null, public type: SourceType = SourceType.Local, packageFolder = "") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to comment this constructor function way better.
| const packageFolder = path.join(fromSourceFolder, moduleFolder, toSourceStart); | ||
|
|
||
| // create two entries and assume file.ts first over file/index.ts | ||
| if (toSourceEnd !== "") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why this if exists is because the first entry assumes that the package folder is toSourceEntries.join(path.sep) in totality. The only thing it could possibly be at this point is {package}/assembly/index.ts. Perhaps we should remove this if statement and copy the second push out of the loop, starting said loop with i = 1.
| function resolveFromUrl(fromSourcePath: string, toSourcePath: string, args: any): Entry[] { | ||
| const isRelativePath = toSourcePath.startsWith("."); | ||
| if (isRelativePath) { | ||
| const resolved = url.resolve(fromSourcePath, toSourcePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this works completely. We should look into url joining algorithms :)
lib/resolver/test.js
Outdated
| #!/usr/bin/env node | ||
| const { resolve, SourceType } = require("./index"); | ||
| const { deepEqual } = require("assert"); | ||
| const snapshots = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Write all the tests here. All we care about is the fact that it generates every possibility correctly, In the case of Assembly folder determination using ascMain, that is done in the resolveSync() function. All we care is that each of the data properties are generated correctly. We could even do JSON.stringify(result, null, 2) and use the diff package do show the difference between the snapshots at test time.
| @@ -0,0 +1,20 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might like to regenerate this with tsc --init. Modify the tsc values there.
|
This should be closed :). |
Todo:
Currently the algorithm is functional but could be optimized.
Supports async package resolution for ide plugins and eventually the compiler too.