refactor: Add debug flag and skip missing deps from the importmap #18
Conversation
|
The reason for writing a custom parseArgs is, the default one from node:until will throw a error for any flag that is not configured in options. But that’s not the case here. Users can pass flag to the loader and flags to their node tasks. So we can’t define all possible flags so a custom one 👍🏽 |
| options: { "debug-node-loader": { alias: "d", type: "boolean", default: false } }, | ||
| }); | ||
|
|
||
| export const isDebuggingEnabled = (): boolean => values["debug-node-loader"] as boolean; |
There was a problem hiding this comment.
--debug is standard, right?
I think we can capture this differently as well (no computeds).
There was a problem hiding this comment.
I wrote the explanation here
#18 (comment)
And the reason for not using --debug is. Users can pass flags similar to this to their own apps. So, --debug is quite generic. If the main app uses it, it can clash with that
There was a problem hiding this comment.
That makes a ton of sense! And we prefer dashes vs camelcase?
| But it is still not widely adopted yet. | ||
| */ | ||
| log.debug(`Failed in resolving ${specifier} in ${cacheMapPath}`); | ||
| return null; |
There was a problem hiding this comment.
What 'value" does null provide here?
There was a problem hiding this comment.
This line here know, that the specifier is failed to resolve from the map and can be skipped.
https://github.com/jspm/node-importmap-http-loader/blob/skip-missing-dependency-from-importmap/src/loader.ts#L39
There was a problem hiding this comment.
I mean that I don't see a reason why returning null is better than nothing. 😬 Sorry I wasn't clearer! 😃
|
@JayaKrishnaNamburu crushing as usual! 💪
Thoughts? |
This PR allows to skip any missing dependencies from the importmap.
And adds a debug flag which disables the logs by default. Much needed since it’s flossing the CI with lot of logs if enabled by default 😅