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 #125

Merged
merged 3 commits into from
Mar 27, 2020
Merged

Update dependencies #125

merged 3 commits into from
Mar 27, 2020

Conversation

HoldYourWaffle
Copy link
Contributor

I have updated all dependencies to the latest version for your convenience.
No major changes were necessary, just some postfix-operators.

There are 5 security warnings remaining (down from ~1600) that require "manual intervention". From what I can see these are caused by transitive dependencies of parcel and lerna.

Copy link
Collaborator

@ci010 ci010 left a comment

Choose a reason for hiding this comment

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

Sorry, but can you rebase your commits so they fulfill the conventional commits? For these two commits, they should be something like

  • chore: update dependencies
  • chore: automatic npm security audit <next-line> ...restOfTheCommitBody

Other than that, I think this PR is good.

I should mention this in contribute.md, but I'm still thinking what kind of test should I use to ensure the commit (commit-lint? Or a gh-action to check commit).

I suggest you can use interactive rebase to fix this: git rebase --interactive <the-sha-of-the-commit-before-your-two-commits>. I believe in your case, it should be git rebase --interactive a034fba5c1f6c04afe06f36f29f1dc132c1f5a95. Then, git will let you modify the git message in a file:

pick <hash> <message>
pick <hash> <message>

And you should change the message and save and quit that file. Then, you will need to git push -f. That will override current branch on github.

@ci010
Copy link
Collaborator

ci010 commented Mar 27, 2020

BTW, would you like to do me a favor, update the got js to 10.7.0 in packages/installer/package.json and it should fix the issue #95

@HoldYourWaffle
Copy link
Contributor Author

HoldYourWaffle commented Mar 27, 2020

Sorry, but can you rebase your commits so they fulfill the conventional commits?

Whoooops I completely forgot about that, although it should definitely be mentioned in the contribution guide.
I don't have experience with setting up "commit linting", but I've seen husky being used to check certain conditions when committing, perhaps it could be useful?

I have now correctly rebased my changes. Even though I already knew how to do it, I greatly admire your willingness to explain for the "less informed" among us. Git can be very confusing at times, and I'm sure that I would've been very confused without your explanation, had I not known about interactive rebase beforehand.

Although,

Then, git will let you modify the git message in a file: (...) And you should change the message and save and quit that file.

This is not correct. You have to change "pick" into "reword" (or 'r' for short) to rename the commits 😉



BTW, would you like to do me a favor, update the got js to 10.7.0 in packages/installer/package.json

This reminded me to update the dependencies for individual modules too, which I have now done.
Unfortunately I encountered some minor issues here:

  1. long can't be updated to v4 because bytebuffer depends on '~3'. Although there aren't any API-changes, npm will download a separate version for bytebuffer because it's a "major version difference". This means that our code would be referencing v4, while bytebuffer references v3. This causes an instanceof-check to fail, which unsurprisingly leads to issues.
    There's already a PR over at bytebuffer to fix this, but it hasn't been merged yet. In the meantime I have rolled back our version to ~3 (in line with bytebuffer).

  2. file-type can't easily be updated to v13 (or later) because they rewrote the API to be asynchronous. This means that the deserializeSync function will be impossible, meaning everywhere this function is used will have to be updated to to asynchronous deserialize (in combination with async/await). This would be a breaking change.
    I think this is a good (and the only) solution, and I would be more than happy to do the grunt work for this if you agree.

I deliberately did these rollbacks in a separate commit to make it easy to revert once "the road is clear" so to speak.

5 security warnings required "manual intervention". It looks like they are caused by transitive dependencies from parcel & lerna
- file-type has an async API from v13 on, more changes would be necessary to update
- long can't be updated to v4 because bytebuffer depends on ~3 (protobufjs/bytebuffer.js#90)
@ci010
Copy link
Collaborator

ci010 commented Mar 27, 2020

I'm curious why the package-lock.json exists at each packages/<module>/package-lock.json? I think that is lerna's problem (Lerna always has this). But I think it's okay for now.

And, yes, we can have another individual patch to fix the file-type and others problem.

@ci010 ci010 merged commit f7f963b into Voxelum:master Mar 27, 2020
@HoldYourWaffle
Copy link
Contributor Author

I'm curious why the package-lock.json exists at each packages//package-lock.json? I think that is lerna's problem

Don't ask me how, but I completely overlooked lerna's existence. I painfully ran install, outdated and update manually in each package, causing the package-lock's to generate. I was already a little confused why they didn't exist before...
I don't see a reason why they shouldn't be there though. There are many benefits to having a lock file, and I found this issue asking a similar question over at lerna.
I'll look more closely into Lerna now, it certainly looks very interesting.

And, yes, we can have another individual patch to fix the file-type and others problem.

Do you want me to ("recursively") update all deserializeSync usages to the async version? In that case, perhaps it would make sense to remove serializeSync as well for consistency.

Also: I just found out I forgot to update the "sub"-modules user/util and nbt/zlib. Can this be done automatically with lerna or do I have to manually update them?

@ci010
Copy link
Collaborator

ci010 commented Mar 27, 2020

Actually,

I'm curious why the package-lock.json exists at each packages//package-lock.json? I think that is lerna's problem

Don't ask me how, but I completely overlooked lerna's existence. I painfully ran install, outdated and update manually in each package, causing the package-lock's to generate. I was already a little confused why they didn't exist before...
I don't see a reason why they shouldn't be there though. There are many benefits to having a lock file, and I found this issue asking a similar question over at lerna.
I'll look more closely into Lerna now, it certainly looks very interesting.

And, yes, we can have another individual patch to fix the file-type and others problem.

Do you want me to ("recursively") update all deserializeSync usages to the async version? In that case, perhaps it would make sense to remove serializeSync as well for consistency.

Also: I just found out I forgot to update the "sub"-modules user/util and nbt/zlib. Can this be done automatically with lerna or do I have to manually update them?

Actually, I'll go over the lerna again and see what is the best practice to setup a existed lerna project. I just check that the others lerna repo does not contain package-lock in repo. It seems harmless now, but let's remove it in next patch.

@HoldYourWaffle
Copy link
Contributor Author

Also: I just found out I forgot to update the "sub"-modules user/util and nbt/zlib. Can this be done automatically with lerna or do I have to manually update them?

Actually, wouldn't it make more sense to make the "sub"-modules depend on `../' and move any remaining (non-transitive) dependencies to the "parent module"?

This makes updating easier in the future, and IMHO makes the "proxy function" of these sub-modules more clear.
I think you're more explicitly saying:

  • This is a module (for example @xmcl/nbt) that does something with these dependencies
  • This little sub-module (zlib) is only here to serve as "proxy" to separate node and browser code.

@ci010
Copy link
Collaborator

ci010 commented Mar 27, 2020

These internal tricky modules' dependencies should already be included in its parent. The dependencies field in it is just to hint the bundle system.

I'm not sure if I understand it correctly. Do you mean the packages/module/sub-module should require packages/module at some point? That will be a circular dependency and might mess up with the bundle system. Currently the dependencies is always look like
packages/module -> packages/module/sub-module.

@HoldYourWaffle
Copy link
Contributor Author

The dependencies field in it is just to hint the bundle system.

Is it necessary though? I feel like it could lead to issues if people (like me) forget to update dependencies.

That will be a circular dependency and might mess up with the bundle system

You're absolutely right, my suggestion doesn't make sense. I feel like there should be a better way to do this though...

@HoldYourWaffle
Copy link
Contributor Author

Actually, wouldn't it be sufficient to move the browser section to the parent module (adjusting the paths of course)?
I haven't done this kind of thing before, but shouldn't that be enough for the bundling system to figure out what file to include?

@HoldYourWaffle HoldYourWaffle deleted the update-dependencies branch March 27, 2020 21:49
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.

2 participants