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

feat: add support for npm 7 and Node 16, migrate to graphiql #1807

Merged
merged 15 commits into from Sep 29, 2021

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Sep 20, 2021

New Pull Request Checklist

Issue Description

Related issue: #1797

Approach

With GraphQL Playground soon to be archived and not receiving support, it's sub-dependancies are not compatible with GraphQL 15. The course of action here is to migrate to GraphQL playgrounds successor, GraphIQL

This fixes install issues on npm 7.

Note: GraphIQL is only compatible with GraphQL=<15.5.0

Closes:

#1797
#1752
#1633
#1639

TODOs before merging

  • Add entry to changelog

@parse-github-assistant
Copy link

parse-github-assistant bot commented Sep 20, 2021

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@mtrezza
Copy link
Member

mtrezza commented Sep 20, 2021

Yay! 🙌 for this PR! That's the way to go forward and address these issues sustainably.

@dblythy
Copy link
Member Author

dblythy commented Sep 20, 2021

@mtrezza circular dependancies throws Error: Cannot find module 'typescript'. Do you have any ideas how to fix this?

@mtrezza
Copy link
Member

mtrezza commented Sep 20, 2021

Without having looked into it, I'd assume the module is referenced somewhere but not imported or defined as dependency. Can also be an upstream issue. Or if you created package-lock using npm 7 with legacy peer dependencies it may be a missing dependency you'd have to manually define.

@mtrezza
Copy link
Member

mtrezza commented Sep 23, 2021

  • This PR uses graphql 15.5.0
  • Parse Server master uses graphql 15.5.2
  • Parse Server 4.10.3 uses graphql 15.4.0

Could there be incompatibilities between these different versions? In other words, do we need a compatibility table dashboard <> server? If so, maybe we should stick with graphql 15.4.0 because Parse Server 4.10.3 is the latest official release?

@dblythy
Copy link
Member Author

dblythy commented Sep 23, 2021

Good idea, I will downgrade the dependancy to 15.4.0

CHANGELOG.md Outdated Show resolved Hide resolved
@mtrezza
Copy link
Member

mtrezza commented Sep 23, 2021

Does this also add support for node 16? Currently dashboard requires "node": ">=12.0.0 <16.0.0". If node 16 is supported, we can remove the <16.0.0 and add node 16 to the CI workflow (I can do that for you if you prefer).

@dblythy
Copy link
Member Author

dblythy commented Sep 23, 2021

I’m honestly not sure, I can check tomorrow

@dblythy dblythy changed the title fix: migrate to graphiql, support npm 7 fix: migrate to graphiql, support npm 7 and Node 16 Sep 24, 2021
@dblythy
Copy link
Member Author

dblythy commented Sep 24, 2021

Bumped some dependancies and some minor refactors to support Node 16

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@mtrezza
Copy link
Member

mtrezza commented Sep 24, 2021

Looks good, I'll just try it out locally with some node versions to see whether it all works fine.

@mtrezza
Copy link
Member

mtrezza commented Sep 24, 2021

For npm 7 it installs, but I get this:

WARNING in ../node_modules/crypto-js/core.js 43:22-39
Module not found: Error: Can't resolve 'crypto' in '.../parse-dashboard/node_modules/crypto-js'

Since webpack bumps from 4.x to 5.x there is also this:

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
        - add a fallback 'resolve.fallback: { "crypto": require.resolve("crypto-browserify") }'
        - install 'crypto-browserify'
If you don't want to include a polyfill, you can use an empty module like this:
        resolve.fallback: { "crypto": false }
 @ ../node_modules/crypto-js/aes.js 4:37-54
 @ ../node_modules/parse/lib/browser/CryptoController.js 9:6-30
 @ ../node_modules/parse/lib/browser/Parse.js 15:47-76
 @ ../node_modules/parse/index.js 1:0-50
 @ ./dashboard/Push/PushDetails.react.js 40:0-26 233:12-25
 @ ./dashboard/Dashboard.js 45:0-51 434:52-63
 @ ./dashboard/index.js 12:0-36 22:50-59

crypto-js is a dependency of the Parse JS SDK:

parse-dashboard@3.0.0
└─┬ parse@3.3.0
  └── crypto-js@4.0.0

The dashboard seems to run fine though, not sure which feature - if any in dashboard - is using crypto-js.

See angular/angular-cli#20819 (comment); not sure whether this is something that needs to be fixed in dashboard or the JS SDK, and whether we can just ignore it for now, or not and have to stick to webpack 4.x.

@dblythy
Copy link
Member Author

dblythy commented Sep 24, 2021

Let me know which way you think is best to approach this. I have no idea about webpack, react, etc, I was just trying to get this package fixed for npm 7.

@mtrezza
Copy link
Member

mtrezza commented Sep 24, 2021

Webpack 4 used to auto-polyfill dependencies that were not meant to run in a browser. crypto-js being one of them, which is a dependency of Parse JS SDK which is a dependency of Parse Dashboard. Webpack 5 doesn't do that anymore, we'd need to manually add poyfills. The issue with polyfills is that they are essentially hacks as a substitute for native libraries that don't support running in browsers.

It seems this has been solved in brix/crypto-js#364. Parse JS SDK 3.3.0 depends on crypto-js@4.0.0, so we'd need to bump it to the latest version, which should fix this. Alternatively move to another library.

Maybe we release a new Parse JS SDK version first, then bump the JS SDK dependency to 3.3.1, then merge this PR. It should be fairly quick.

@mtrezza
Copy link
Member

mtrezza commented Sep 24, 2021

I release Parse JS SDK 3.3.1 Can you bump parse to 3.3.1 in this PR? Then we should good to merge.

package.json Outdated Show resolved Hide resolved
@dblythy
Copy link
Member Author

dblythy commented Sep 28, 2021

Should be good to go!

@mtrezza
Copy link
Member

mtrezza commented Sep 29, 2021

Really that easily? Does this build fine with npm 7 and 6?

@dblythy
Copy link
Member Author

dblythy commented Sep 29, 2021

Yep, worked fine for me!

@mtrezza
Copy link
Member

mtrezza commented Sep 29, 2021

I can confirm, it works for me locally too with npm 6 and 7.

Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Amazing work! This fix will surely be a relief for many developers. Given the Node 16 / npm 7 support, I bumped this up to a feature commit.

@mtrezza mtrezza changed the title fix: migrate to graphiql, support npm 7 and Node 16 feat: migrate to graphiql, support npm 7 and Node 16 Sep 29, 2021
@mtrezza mtrezza changed the title feat: migrate to graphiql, support npm 7 and Node 16 feat: add support for npm 7 and Node 16, migrate to graphiql Sep 29, 2021
@mtrezza mtrezza merged commit b61fc7f into parse-community:master Sep 29, 2021
parseplatformorg pushed a commit that referenced this pull request Sep 29, 2021
# [3.1.0](3.0.0...3.1.0) (2021-09-29)

### Features

* add support for npm 7 and Node 16, migrate to graphiql ([#1807](#1807)) ([b61fc7f](b61fc7f))
@parseplatformorg
Copy link
Contributor

🎉 This pull request has been released in version 3.1.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Sep 29, 2021
@mtrezza
Copy link
Member

mtrezza commented Sep 29, 2021

Congrats @dblythy, your PR was the first one across Parse to be released automatically 🙌

@dblythy dblythy deleted the fix-graphql branch September 29, 2021 23:49
@LMBernardo
Copy link

@dblythy
This has been a constant pain for me, I can't thank you enough for this PR. Greatly appreciated! Throw a crypto address on your profile and I'll buy you a beer. 😁 🍻

@dblythy
Copy link
Member Author

dblythy commented Sep 30, 2021

@LMBernardo your generosity is greatly appreciated, however all of the core team are constantly working at improving our packages, and I can’t accept your offer simply because I addressed a neglected, difficult to isolate problem. If you would like to support our work, you are welcome to contribute to our OpenCollective 😊

Enjoy the update and let us know if you run into any other problems, or have any suggestions for improvements!

@stage88
Copy link

stage88 commented Sep 30, 2021

Big win guys. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants