-
Notifications
You must be signed in to change notification settings - Fork 392
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
feat(compiler): compiler returns import locations #139
feat(compiler): compiler returns import locations #139
Conversation
getLocations(code: string): ImportLocation[] { | ||
const locations: ImportLocation[] = []; | ||
|
||
if (!code) { |
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.
Not needed.
@@ -0,0 +1,66 @@ | |||
export interface ImportLocation { | |||
name: string; | |||
location: { |
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.
Use the Location
interface instead of inlining the type here.
const QUOTE_LENGTH = 1; | ||
const MODULE_IMPORT_REGEX = /(?<=define\()('.*?', )\[('.*?')\]/; | ||
|
||
export class ImportLocationCollector { |
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.
Why using a class since the only exposed method getLocation
is stateless?
In this context, a simple function would make more sense.
return { | ||
diagnostics, | ||
code, | ||
map: null, | ||
metadata: metadataCollector.getMetadata() | ||
metadata: metadataCollector.getMetadata(), | ||
importLocations, |
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.
What is the motivation behind adding importLocations
property to the BundleReport
instead of Metadata
?
export interface BundleReport { | ||
code: string; | ||
diagnostics: Diagnostic[]; | ||
map: null; | ||
metadata: BundleMetadata; | ||
importLocations: ImportLocation[]; |
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.
Use ModuleImportLocations
instead of ImportLocation[]
since the same type is defined below.
// 1. full match | ||
// 2. filename | ||
// 3. imports | ||
if (!matches || matches.length < EXPECTED_MATCH_LENGTH) { |
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 it worth logging a warning if the getLocations
can't even match the define(
prefix for AMD. Since the compiler expects this specific format.
Benchmark comparisonBase commit:
|
@pmdartus please review when you get a chance |
Details
Compiler's bundle result object will return list of module import locations
Does this PR introduce a breaking change?