Skip to content
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

Update dependencies #481

Merged
merged 8 commits into from
Aug 25, 2020
Merged

Update dependencies #481

merged 8 commits into from
Aug 25, 2020

Conversation

kinow
Copy link
Member

@kinow kinow commented Aug 19, 2020

This is a small change with no associated Issue.

I used ncu to get a list of dependencies that had updates available. Then went through these dependencies individually, updating each one and checking if the build was working OK.

vuetify was not updated as it is being updated in #466.

Updating eslint-config-vuetify resulted in some weird errors in yarn install:

error An unexpected error occurred: "could not find a copy of eslint to link in /home/kinow/Development/python/workspace/cylc-ui/node_modules/eslint-config-vuetify/node_modules".

So I reverted it and will take care of that dependency later (it is a dev dependency).

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Already covered by existing tests.
  • Appropriate change log entry included.
  • No documentation update required.

@kinow kinow self-assigned this Aug 19, 2020
@kinow kinow added this to the 0.3 milestone Aug 19, 2020
@kinow
Copy link
Member Author

kinow commented Aug 19, 2020

Huh, more dependencies missing now. Some core-js, eslint-plugin-import... :sigh:

Always an adventure updating JS dependencies 🤓

package.json Outdated
"@lumino/datagrid": "^0.12.0",
"@lumino/default-theme": "^0.4.4",
"@lumino/widgets": "^1.13.4",
"apollo-boost": "^0.4.9",
Copy link
Member Author

Choose a reason for hiding this comment

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

When we first added Apollo, we used apollo-boost. Which is just the apollo-client wrapped by several utility functions, and features like caching.

Later, after realizing we didn't need any of the features in apollo-boost, I started moving the WorkflowService to use just ApolloClient. That also became necessary as we started customizing the client for the WebSockets.

I will have to confirm, but I believe I have finished getting rid of apollo-boost, but added the dependencies for the non-apollo-boost setting in the devDependencies.

If so, we can remove the Apollo dependencies from devDependencies, move them up here, and maybe reduce our list of dependencies, as ApolloBoost includes more code/dependencies.

"cytoscape-expand-collapse": "^3.1.2",
"cytoscape-node-html-label": "^1.1.5",
"cytoscape-expand-collapse": "^4.0.0",
"cytoscape-node-html-label": "^1.2.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm updating the Graph dependencies, assuming they won't require us to change anything in the Graph component or view 👍 if they demand further work, I will remove these changes to simplify the PR, since we may choose a different library for handling Graphs in Cylc UI.

Copy link
Member

@hjoliver hjoliver Aug 19, 2020

Choose a reason for hiding this comment

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

I think we may want to remove the graph view next release, pending @oliver-sanders massive re-do. Probably not good to have a barely-usable POC graph view with wrong look-and-feel alongside our nice (and now performant, post deltas) tree view. (n-distance windows might save graph view performance, but still have the other problems...).

Copy link
Member Author

Choose a reason for hiding this comment

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

At least removing dependencies and code is easier. Just need to remember to remove not only cytoscape- dependencies, but also the tippy I think. Would be nice if there were dependency groups in NPM, or if at least we could add comments to package.json (JSON limitation)

@kinow
Copy link
Member Author

kinow commented Aug 19, 2020

eslint-plugin-import missing after updating dependencies. Added with yarn add -D eslint-plugin-import.

@kinow
Copy link
Member Author

kinow commented Aug 19, 2020

Now vue-markdown is giving me issues after updating the other dependencies. Related to this issue: miaolz123/vue-markdown#18

Looks like vue-markdown require us to include another dependency now: yarn add babel-runtime --save 🤷‍♂️

@kinow
Copy link
Member Author

kinow commented Aug 19, 2020

I can now build it with yarn run build:watch, but one warning in the ./srv/views/Mutations.vue:

"export 'introspectionQuery' was not found in 'graphql'

🤔

@codecov-commenter
Copy link

codecov-commenter commented Aug 19, 2020

Codecov Report

Merging #481 into master will decrease coverage by 8.46%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #481      +/-   ##
==========================================
- Coverage   69.90%   61.43%   -8.47%     
==========================================
  Files          58       42      -16     
  Lines        1266      918     -348     
  Branches       78       42      -36     
==========================================
- Hits          885      564     -321     
+ Misses        361      341      -20     
+ Partials       20       13       -7     
Flag Coverage Δ
#e2e ?
#unittests 61.43% <0.00%> (-0.44%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/views/Mutations.vue 37.87% <0.00%> (-4.55%) ⬇️
src/mixins/index.js 0.00% <0.00%> (-100.00%) ⬇️
src/mixins/treeview.js 0.00% <0.00%> (-100.00%) ⬇️
src/router/index.js 0.00% <0.00%> (-96.67%) ⬇️
src/main.js 0.00% <0.00%> (-91.67%) ⬇️
src/components/cylc/workflow/lumino-widget.js 0.00% <0.00%> (-84.22%) ⬇️
src/components/cylc/workflow/Lumino.vue 30.00% <0.00%> (-66.67%) ⬇️
src/components/cylc/graph/index.js 0.00% <0.00%> (-42.86%) ⬇️
src/store/workflows.module.js 63.63% <0.00%> (-36.37%) ⬇️
src/components/cylc/tree/TreeItem.vue 69.23% <0.00%> (-26.93%) ⬇️
... and 24 more

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 92db6fd...d88bed4. Read the comment docs.

@kinow
Copy link
Member Author

kinow commented Aug 19, 2020

I can now build it with yarn run build:watch, but one warning in the ./srv/views/Mutations.vue:

"export 'introspectionQuery' was not found in 'graphql'

thinking

Tree view OK.

Graph view OK.

Mutations view is now broken. Looking into that.

image

From the warning message, it looks like the GraphQL library stopped exporting introspectionQuery?

package.json Outdated
"sinon": "^7.5.0",
"standard": "^14.3.1",
"subscriptions-transport-ws": "^0.9.16",
"lodash": "^4.17.20",
Copy link
Member Author

Choose a reason for hiding this comment

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

I think lodash is actually a runtime dependency. Maybe it's working now if another dependency is loading it (the Apollo libraries do). But this needs to be moved to the dependencies and removed from the devDependencies.

@kinow
Copy link
Member Author

kinow commented Aug 19, 2020

Fixed build, no more warnings, but for some reason the Mutations are not loading now.

image

The view /#/mutations works with no issues however. So might be something with the wrappers, or the Lumino tab that affects only the mutations.

@kinow
Copy link
Member Author

kinow commented Aug 19, 2020

The Mutations view was wrapped in a skeleton loader that had a isLoading flag always set to true. Funny that I think it was supposed to happen on master too.

Anyhoo, have modified it to leave the loading to the Mutations view, as that view is using another flag for whether it is loading or not.

For tomorrow:

  • move lodash to dependencies, and remove from dev dependencies
  • move apollo dependencies to dependencies, and remove them from dev dependencies
  • another manual test to make sure there are no regressions
  • change log

Then it should be ready for review

@oliver-sanders
Copy link
Member

Looks good to me.

@kinow kinow marked this pull request as ready for review August 19, 2020 22:32
@kinow
Copy link
Member Author

kinow commented Aug 19, 2020

PR ready for review.

The master branch produces 8MB of assets.

image

This branch produces 7.9MB.

image

Most likely related to updates in libraries that have either reduced their code size, or did changes that permit better tree-shaking. Could also be related to the removal of ApolloBoost, replaced by the fine-grained Apollo libraries (ApolloBoost just wraps several libraries into a library for convenience, plus some constructors to get clients with cache, etc).

Bruno

<component
is="VSkeletonLoader"
:ref="widgetId"
:loading="isLoading"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is always true if you have nothing in the UI but the mutation. With Graph and Tree views, the subscriptions when started can change the view's isLoading state.

But the mutation widget doesn't trigger any subscription.

So instead we simply add a Mutation view. The view has a loading state, and displays "Loading..." immediately when loaded. Furthermore, with work to add mutations in the right places, this view might be hidden or removed I guess.

@@ -39,7 +39,7 @@ along with this program. If not, see <http://www.gnu.org/licenses/>.

<script>
import gql from 'graphql-tag'
import { introspectionQuery, print } from 'graphql'
import { getIntrospectionQuery as getGraphQLIntrospectionQuery, print } from 'graphql'
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed in GraphQL 15. Luckily they didn't remove the functionality.

Note that we already have a function in this module called getIntrospectionQuery, by coincidence. So if we imported like this, we could end up with weird build issues (e.g. recursively calling itself instead of calling the function from the graphql module). So I've renamed the import 👍

@kinow
Copy link
Member Author

kinow commented Aug 19, 2020

Rebased

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Tested in offline mode, graph broken but that might be expected.

I'm getting a lot of "incorrect peer dependency" warnings during installation (though I got some of these before):

warning "@lumino/datagrid > @lumino/[email protected]" has unmet peer dependency "[email protected]".
warning "apollo-client > [email protected]" has incorrect peer dependency "graphql@^0.11.3 || ^0.12.3 || ^0.13.0 || ^14.0.0".
warning "apollo-link > [email protected]" has incorrect peer dependency "graphql@^0.11.0 || ^0.12.0 || ^0.13.0 || ^14.0.0".
warning " > [email protected]" has unmet peer dependency "prop-types@>=15.5.0".
warning " > @babel/[email protected]" has unmet peer dependency "@babel/core@^7.0.0-0".
warning "@babel/plugin-proposal-class-properties > @babel/[email protected]" has unmet peer dependency "@babel/core@^7.0.0".
warning " > @cypress/[email protected]" has unmet peer dependency "cypress@*".
warning " > @cypress/[email protected]" has unmet peer dependency "@babel/core@^7.0.1".
warning " > @cypress/[email protected]" has unmet peer dependency "@babel/preset-env@^7.0.0".
warning " > @cypress/[email protected]" has unmet peer dependency "babel-loader@^8.0.2".
warning "@vue/cli-plugin-eslint > [email protected]" has incorrect peer dependency "eslint@>=1.6.0 <7.0.0".
warning " > [email protected]" has unmet peer dependency "eslint-plugin-promise@>=4.2.1".
warning " > [email protected]" has incorrect peer dependency "eslint@^6.7.2".
warning " > [email protected]" has incorrect peer dependency "eslint@^5.0.0 || ^6.0.0".

@kinow
Copy link
Member Author

kinow commented Aug 20, 2020

Tested in offline mode, graph broken but that might be expected.

Yup, the graph never worked in offline mode. That's because the mocked WorkflowService only returns the tree data.

I'm getting a lot of "incorrect peer dependency" warnings during installation (though I got some of these before):

Yup, caused by the move to Graphql 15. Most of our dependencies have some ranges like graphql 13 | 14, or something similar to that. But apparently everything that we use is still working.

But if you prefer to reduce the risk by moving it down to 14 I can downgrade it too (takes just a few minutes as I'm deleting the node_modules when updating just to make sure I don't have any vestige of previous installations)

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Yup, caused by the move to Graphql 15.

Ok, if you've got a handle on what's causing it, looks like the warnings are harmless.

I used `ncu` to get a list of dependencies that had updates available. Then went through these dependencies individually, updating each one and checking if the build was working OK.

`vuetify` was not updated as it is being updated in cylc#466.

Updating `eslint-config-vuetify` resulted in some weird errors in `yarn install`:

error An unexpected error occurred: "could not find a copy of eslint to link in /home/kinow/Development/python/workspace/cylc-ui/node_modules/eslint-config-vuetify/node_modules".

So I reverted it and will take care of that dependency later (it is a dev dependency).
@kinow
Copy link
Member Author

kinow commented Aug 20, 2020

Rebased

@hjoliver hjoliver merged commit 292d44e into cylc:master Aug 25, 2020
@kinow kinow deleted the update-dependencies branch August 25, 2020 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants