[dev-tool] Allow relative-path imports in sample files. #14665
Merged
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
witemple-msft
added
EngSys
witemple-msft
requested review from
mikeharder and
praveenkuttappan
as code owners
deyaaeldeen
approved these changes
Apr 1, 2021
Comment on lines
+119
to
+124
| if (nodeBuiltins.includes(moduleSpecifier)) return false; | ||
|
|
||
| // This seems like a reasonable test for "is a relative path" as long as | ||
| // absolute path imports are forbidden. | ||
| const isRelativePath = /^\.\.?\//.test(moduleSpecifier); | ||
| return !isRelativePath; |
There was a problem hiding this comment.
NIT:
Suggested change
| if (nodeBuiltins.includes(moduleSpecifier)) return false; | |
| // This seems like a reasonable test for "is a relative path" as long as | |
| // absolute path imports are forbidden. | |
| const isRelativePath = /^\.\.?\//.test(moduleSpecifier); | |
| return !isRelativePath; | |
| return !(nodeBuiltins.includes(moduleSpecifier) || | |
| // This seems like a reasonable test for "is a relative path" as long as | |
| // absolute path imports are forbidden. | |
| /^\.\.?\//.test(moduleSpecifier)); |
Not sure about formatting though :)
jay-most
pushed a commit
to jay-most/azure-sdk-for-js
that referenced
this pull request
Apr 26, 2021
openapi-sdkautomation bot
pushed a commit
to AzureSDKAutomation/azure-sdk-for-js
that referenced
this pull request
Jun 8, 2021
Changes in operation ids (Azure#14665) * manual changes for swagger * changes for long-running operations * changes for validate for backup and restore * removing 204 from operationResults * changes for a monitoring * adding format * removing date-time from duration * changing to readonly * changes for listRPs * fixes swagger * reverting readonly for scheduleTimes * fixes for checkgates * prettier fixes * cahnges for friendlyname checks * fixing gates * removing tracking-via * model validation fixes * powershell changes * adding stable version * changes for PatchResourceRequestInput * change for req fields * changes for description * changes * Updated swagger json for item level restore target info objects * Fixed spelling errors * Added missing required properties * removed auth credentials property * prettier fixes * Prettier fix * pr comments fixes * fixing readme * changes for go readme * Removed x-ms-flatten property (#2) * removed flatten prop * removing x-ms-flatten * removing friendlyName from req prop Co-authored-by: Mayank Aggarwal <mayaggar@microsoft.com> * changes in operation ids (#3) * changes in operation ids * changes * changes Co-authored-by: Mayank Aggarwal <mayaggar@microsoft.com> * lint and model vaidations * changes for prettier * changing version in example Co-authored-by: Mayank Aggarwal <mayaggar@microsoft.com> Co-authored-by: FAREAST\zakinkh <zakinkh@microsoft.com>
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.
This is a little oversight when analyzing the sample files' imports. Relative path imports need to be filtered out when considering dependencies. We were already filtering node builtin modules, so I abstracted the logic to a helper function
isDependency.It's difficult to support absolute path imports in the samples without doing a lot more work involving simulating node module resolution and adding a lot more logic to the output structure for TS and JS. Plus, I'm not convinced that absolute path imports make a lot of sense within the sample code anyway. So, relative path imports need to start with one or two dots and a forward slash, i.e. the regexp
/^\.\.?\//.I also made the dependency diagnostic error refer to the possibility that "./" is missing for relative imports.
We found this because @vishnureddy17 is importing JSON files as modules in the samples for digital-twins-core, and he was seeing messages such as:
And this should be a well-supported use-case. Since I don't think there's any harm in doing it, I also added
resolveJsonModules: trueby default in the sample tsconfig.