Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Make missing token error more actionable #322

Merged
merged 4 commits into from
Oct 12, 2018
Merged

Conversation

lhorie
Copy link
Contributor

@lhorie lhorie commented Oct 12, 2018

TL;DR: People have been running into an issue where having multiple versions of a same plugin would cause the DI system to throw errors about missing dependencies. The error was cryptic and difficult to troubleshoot.

This PR exposes stack information about the missing token as well as potential problematic duplicated tokens, and it offers actionable suggestions to fix the issue.

I labeled this PR w/ the feature flag because this diff adds a stack field to Token

// Before
Error: Could not resolve token: "A", which is required by plugins registered with tokens: "UnnamedPlugin". Did you forget to register a value for "A"?
    at resolveToken (/Users/lhorie/Documents/aaa/node_modules/fusion-core/src/base-app.js:202:15)
    at resolvePlugin (/Users/lhorie/Documents/aaa/node_modules/fusion-core/dist/index.js:544:31)
    at resolveToken (/Users/lhorie/Documents/aaa/node_modules/fusion-core/dist/index.js:560:20)
    at App.resolve (/Users/lhorie/Documents/aaa/node_modules/fusion-core/dist/index.js:590:7)
    at App.resolve (/Users/lhorie/Documents/aaa/node_modules/fusion-core/dist/index.js:895:20)
    at App.callback (/Users/lhorie/Documents/aaa/node_modules/fusion-core/dist/index.js:899:12)
    at reload (/Users/lhorie/Documents/aaa/node_modules/fusion-cli/entries/server-entry.js:90:1)
    at 
    at process._tickCallback (internal/process/next_tick.js:189:7)
    at evalScript (bootstrap_node.js:482:13)

// After
Error: A plugin depends on a token, but the token was not registered

Required token: A
This token is required by plugins registered with tokens: "Foo", "Bar"
Error
    at new TokenImpl (/Users/lhorie/Documents/aaa/node_modules/fusion-core/dist/index.js:45:18)
    at createToken (/Users/lhorie/Documents/aaa/node_modules/fusion-core/dist/index.js:55:10)
    at Module../src/main.js (/Users/lhorie/Documents/aaa/src/main.js:10:1)
    at __webpack_require__ (/Users/lhorie/Documents/aaa/webpack/bootstrap:682:1)
    at fn (/Users/lhorie/Documents/aaa/webpack/bootstrap:59:1)
    at Module../node_modules/fusion-cli/entries/server-entry.js (/Users/lhorie/Documents/aaa/node_modules/fusion-cli/entries/server-entry.js:46:1)
    at __webpack_require__ (/Users/lhorie/Documents/aaa/webpack/bootstrap:682:1)
    at fn (/Users/lhorie/Documents/aaa/webpack/bootstrap:59:1)
    at Object.0 (/Users/lhorie/Documents/aaa/.fusion/dist/development/server/server-main.js:1518:18)
    at __webpack_require__ (/Users/lhorie/Documents/aaa/webpack/bootstrap:682:1)

Different tokens with the same name were detected:

A
Error
    at new TokenImpl (/Users/lhorie/Documents/aaa/node_modules/fusion-core/dist/index.js:45:18)
    at createToken (/Users/lhorie/Documents/aaa/node_modules/fusion-core/dist/index.js:55:10)
    at Module../src/main.js (/Users/lhorie/Documents/aaa/src/main.js:9:1)
    at __webpack_require__ (/Users/lhorie/Documents/aaa/webpack/bootstrap:682:1)
    at fn (/Users/lhorie/Documents/aaa/webpack/bootstrap:59:1)
    at Module../node_modules/fusion-cli/entries/server-entry.js (/Users/lhorie/Documents/aaa/node_modules/fusion-cli/entries/server-entry.js:46:1)
    at __webpack_require__ (/Users/lhorie/Documents/aaa/webpack/bootstrap:682:1)
    at fn (/Users/lhorie/Documents/aaa/webpack/bootstrap:59:1)
    at Object.0 (/Users/lhorie/Documents/aaa/.fusion/dist/development/server/server-main.js:1518:18)
    at __webpack_require__ (/Users/lhorie/Documents/aaa/webpack/bootstrap:682:1)

You may have multiple versions of the same plugin installed.
Ensure that `yarn list [the-plugin]` results in one version, and use a yarn resolution or merge package version in your lock file to consolidate versions.


    at resolveToken (/Users/lhorie/Documents/aaa/node_modules/fusion-core/src/base-app.js:164:17)
    at resolvePlugin (/Users/lhorie/Documents/aaa/node_modules/fusion-core/dist/index.js:522:31)
    at resolveToken (/Users/lhorie/Documents/aaa/node_modules/fusion-core/dist/index.js:538:20)
    at App.resolve (/Users/lhorie/Documents/aaa/node_modules/fusion-core/dist/index.js:568:7)
    at App.resolve (/Users/lhorie/Documents/aaa/node_modules/fusion-core/src/server-app.js:25:12)
    at App.callback (/Users/lhorie/Documents/aaa/node_modules/fusion-core/dist/index.js:877:12)
    at reload (/Users/lhorie/Documents/aaa/node_modules/fusion-cli/entries/server-entry.js:90:1)
    at 
    at process._tickCallback (internal/process/next_tick.js:189:7)
    at evalScript (bootstrap_node.js:482:13)

@lhorie lhorie added the feature label Oct 12, 2018
Copy link
Contributor

@KevinGrandon KevinGrandon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think this helps the duplicate token case, but makes the missing token case much harder to debug. The dependentToken case was very useful to know what tokens depended on this token, so you know where to look for documentation. Anyway we can keep both?

Copy link
Contributor

@KevinGrandon KevinGrandon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, this error message is extremely useful when forgetting to register a token:

Error: Could not resolve token: "ApolloClientEndpointToken", which is required by plugins registered with tokens: "ApolloClientToken". Did you forget to register a value for "ApolloClientEndpointToken"?

@lhorie
Copy link
Contributor Author

lhorie commented Oct 12, 2018

@KevinGrandon I incorporated the old message into the new one. It now says This token is required by plugins registered with tokens: "Foo", "Bar". See updated code section above

@codecov
Copy link

codecov bot commented Oct 12, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@e9e44d0). Click here to learn what that means.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #322   +/-   ##
=========================================
  Coverage          ?   93.31%           
=========================================
  Files             ?       18           
  Lines             ?      434           
  Branches          ?       85           
=========================================
  Hits              ?      405           
  Misses            ?       16           
  Partials          ?       13
Impacted Files Coverage Δ
src/create-token.js 100% <100%> (ø)
src/base-app.js 91.02% <69.23%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9e44d0...ca37232. Read the comment docs.

Copy link
Contributor

@KevinGrandon KevinGrandon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is definitely starting to get better. I would love some kind of tree-view of all registered tokens...

@lhorie
Copy link
Contributor Author

lhorie commented Oct 12, 2018

Yeah, I'm thinking what's the best way to expose that tree. I'm going to merge this for now, and look into a DI graph viz separately

@lhorie lhorie merged commit 267da46 into master Oct 12, 2018
@KevinGrandon KevinGrandon deleted the improve-missing-token-error branch October 12, 2018 20:26
@KevinGrandon KevinGrandon mentioned this pull request Oct 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants