Skip to content

harmonize npm packages #4194

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

Closed
wants to merge 19 commits into from
Closed

Conversation

haraldschilly
Copy link
Contributor

@haraldschilly haraldschilly commented Oct 25, 2019

Description

this upgrades npm packages to be more uniformly across packages than before. the remaining ones look like that. I plan to deal with them in a later patch.

this depends on PR 4191

Regular Code Modules
                              cocalc-ui      jupyter        puppeteer      smc-hub        smc-project    smc-util       smc-util-node  smc-webapp     src
async                                                                      ^1.4.2         ^1.5.0         ^1.4.2         ^1.4.2         ^2.6.1
async-await-utils                                                          ^3.0.1         ^2.0.4         ^2.0.4                        ^2.0.4
coffee-loader                                                              ^0.7.2         ^0.9.0                                                      ^0.9.0
coffeelint                                                                                               ^1.13.0                                      ^2.1.0
formidable                                                                 ^1.0.17        ^1.1.1                                       ^1.2.1
jsdom                                                                      ^11.3.0                                      ^7.2.2
node-cjsx                                                                  ^2.0.0                                                                     ^1.0.0
serve-index                                                                ^1.9.1         ^1.7.2
temp                                                                       ^0.8.3         ^0.8.3                        ^0.8.3                        ^0.9.0
winston                                                                    ^2.4           ^1.1.1                        ^1.1.1

Testing Steps

well, mostly minor updates, but all aspects are affected and hence this needs some thorough checks.
In particular, these aspects got upgrades

  • uploading files (formidable)
  • sanitizing html via jquery + jsdom (tests pass, though)
  • lru-cache in project, that's for caching kernel data → test jupyter
  • little changes to smc-util/sync/editor/db/doc.ts to avoid TS errors.

Relevant Issues

Checklist:

  • No debugging console.log messages.
  • All new code is actually used.
  • Non-obvious code has some sort of comments.

Front end:

  • Restart at least one project and check its ~/.smc/local_hub/local_hub.log
  • Completely restart Webpack with ./w in /src
  • Completely restart the hub by killing and restarting ./start_hub.py in /src/dev/project
  • Screenshots if relevant.

@haraldschilly haraldschilly force-pushed the npm-harmonize-updates-part-1 branch from d9f009f to 3a7d4ce Compare October 25, 2019 13:14
@williamstein
Copy link
Contributor

This is soooo awesome!

@haraldschilly
Copy link
Contributor Author

yep, it lets us iterate "towards zero" inconsistencies.

but watch out, I had to make some changes to the sync code – otherwise TS isn't happy. this needs your second pair of 👀

@haraldschilly haraldschilly changed the title harmonize npm packages / part 1 harmonize npm packages Oct 28, 2019
@haraldschilly
Copy link
Contributor Author

so, this is not ready to merge at all. I was optimistic this evening, but there are a lot of problems in the details.

Interestingly, updating "async" to a version above 1 causes troubles in the hub. That's a hint that we have some issues with that library (calling cb twice or whatever). breaking changes

I'll try to sort this out, otherwise I'll go back to an old version.

@williamstein
Copy link
Contributor

calling cb twice or whatever

Yes, I see that now "Calling a callback more than once is considered an error, and an error will be thrown.". I would very much like to know if there any cases in our code where we are using async and call a callback more than once, since that would surely be very bad.

@haraldschilly
Copy link
Contributor Author

haraldschilly commented Oct 29, 2019

ok, I finally realized the problem: mixing of callback vs. async/await.

There are several cases where a async.series array contains functions, which use await. They're promises, not traditional functions! So, for these, newer versions of that async library switch to use the async/await pattern instead of calling these functions with a callback. This makes a lot of sense, but breaks what we have. With a few tweaks (using awaiting.callback and removing cb() / throwing an error) I can get the hub to start up fine, but there are many more cases to search for and take care of.

I think we should thoroughly test this without upgrading async and then think about the next steps.

@haraldschilly
Copy link
Contributor Author

hmm ... I don't know why, but I can't open any files. Weird...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants