fix: #1210 - Add error handling for missing dependencies

[mohammedsahl]

Summary
This PR introduces an error handling method for missing dependencies as described in issue #1210 . onwarn intercepts warning messages and when message.code === 'UNRESOLVED_IMPORT' it means there is an unresolved or missing dependencies. Prior to this PR, a warning message would be logged to the console. The changes made throw an error instead, which can be handled by the appropriate error handler

Before:

lib/plugins/front-matter.min.js
lib/plugins/front-matter.js
lib/plugins/search.min.js
lib/plugins/search.js
'medium-zoom' is imported by src\plugins\zoom-image.js, but could not be resolved – treating it as an external dependency
'medium-zoom' is imported by src\plugins\zoom-image.js, but could not be resolved – treating it as an external dependency
lib/plugins/zoom-image.min.js
lib/plugins/zoom-image.js
No name was provided for external module 'medium-zoom' in output.globals – guessing 'mediumZoom'
No name was provided for external module 'medium-zoom' in output.globals – guessing 'mediumZoom'
lib/docsify.js
lib/docsify.min.js

After:

lib/plugins/front-matter.js
lib/plugins/front-matter.min.js
Error: Could not resolve module medium-zoom
    at Object.onwarn (C:\Users\simpl\Desktop\docsify\build\build.js:35:17)
    at Graph.onwarn (C:\Users\simpl\Desktop\docsify\node_modules\rollup\dist\shared\index.js:87:25)
    at Graph.warn (C:\Users\simpl\Desktop\docsify\node_modules\rollup\dist\rollup.js:17668:14)
    at ModuleLoader.handleMissingImports (C:\Users\simpl\Desktop\docsify\node_modules\rollup\dist\rollup.js:17239:24)
    at ModuleLoader.<anonymous> (C:\Users\simpl\Desktop\docsify\node_modules\rollup\dist\rollup.js:17288:26)
    at Generator.next (<anonymous>)
    at fulfilled (C:\Users\simpl\Desktop\docsify\node_modules\rollup\dist\rollup.js:40:28)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! docsify@4.11.4 build:js: `cross-env NODE_ENV=production node build/build.js`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the docsify@4.11.4 build:js script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\simpl\AppData\Roaming\npm-cache\_logs\2020-06-19T19_26_30_901Z-debug.log

Testing methdology

1. Run npm install; npm run build:js. The build should successful. This step is to ensure that there are no problems before deleteing a dependency

2. Take note of the dependencies in package.json
   https://github.com/docsifyjs/docsify/blob/13f91a4fcb52fb7757e7ecbb4262fd89247dabdd/package.json#L55-70
   We will select medium-zoom to be the test dependecy

3. Delete the appropriate directory from node_modules (node_modules/medium-zoom for our testing purposes)

4. Execute npm run build:js. An error message must be thrown, similar to the one above, labeled After

5. Run npm install; npm run build:js. The build should successful

What kind of change does this PR introduce? (check at least one)

• Bugfix
• Feature
• Code style update
• Refactor
• Docs
• Build-related changes
• Other, please describe:

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

• Yes
• No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

• When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

• Chrome
• Firefox
• Safari
• Edge
• IE

If adding a new feature, the PR's description includes:

• A convincing reason for adding this feature
• Related documents have been updated
• Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

---

• DO NOT include files inside lib directory.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/gd5b2661n
✅ Preview: https://docsify-previe-git-fork-mlh-fellowship-1210-missing-depe-065e64.docsify-core.vercel.app

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3a22fa4:

Sandbox	Source
nervous-curran-twvx3	Configuration

[anikethsaha]

LGTM

Thanks

[trusktr]

👍

[anikethsaha]

@trusktr is it fixing/closing the issue that is mentioned ?

[jhildenbiddle]

What other information is passed with the message object to onWarn()?

Ideally, we would check the path to verify that the unresolved import is something we want to import locally (i.e. from the src directory). Otherwise, this change will throw an error if we ever release an ES6 module that imports external resources. In other words, this change will address the issue today, but it could cause builds to break down the road.

[anikethsaha]

currently we need to check for modules that are installed.
if es6 comes, I guess then we want to check for both dep and local modules as well.

[mohammedsahl]

@jhildenbiddle A WarningHandler is also passed into the function as shown here

Warnings objects have, at a minimum, a code and a message property, allowing you to control how different kinds of warnings are handled. Other properties are added depending on the type of warning.

[jhildenbiddle]

@mohammedsahl --

Let's verify if onwarn() is called when a dependency is explicitly marked as external using rollup's external configuration option. This will help us determine how our custom onwarn() callback should behave.

Using the Before example from above as an example, try adding 'medium-zoom' (the name of the module that rollup cannot resolve) to the external array:

external: [
  'medium-zoom'
]

If onwarn() is not called, then rollup is smart enough to check the external array and realize that the unresolved module has been explicitly marked as an external dependency. This means we don't have to worry about issues with external dependencies in the future since adding them to the rollup's external array (which should be done anyway) will prevent onwarn() from being called. This also means that the onwarn() callback should always throw an error if message.code === UNRESOLVED_IMPORT (as it does now) because we know 1) either a module was not added to the external configuration or 2) a non-external dependency can not be resolved. The only change I would make here is to update the error message to handle both scenarios: Could not resolve module [message.source]. Try running 'npm install' or using rollup's 'external' option if this is an external dependency..

If onwarn() is called, then rollup is making no distinction between external dependencies explicitly listed in the external configuration and modules it simply cannot resolve. This means our onwarn() callback needs handle both scenarios by checking the message.source value against the external value(s) (or function -- see rollup docs) to determine if an error has actually occurred. The goal is to make sure we aren't throwing an error for a properly configured external dependency found in rollup's external configuration.

Finally, if the message object returned to onwarn() contains location information (per the rollup docs), let's include the path to the offending file in the console output (similar to rollup's default message). This will make life easier for devs if troubleshooting is necessary.

[mohammedsahl]

@jhildenbiddle
Turns out that marking a dependency as external does not trigger onwarn(). I've made changes to the error message as you've suggested. Also added information about where the dependency is being imported

[jhildenbiddle]

Excellent!

Good to see rollup is doing the right thing. Much easier than handling the comparison with external values ourselves. I also think we’ve saved a future dev some headaches if/when core leveraged external dependencies.

Nice work, @mohammedsahl! :)