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

Bumping all packages that have minor and patch updates available #4453

Merged
merged 9 commits into from
Oct 31, 2017

Conversation

vitalbone
Copy link
Contributor

Description of changes

Updating all packages that aren't a major version upgrade.

Related issues (if any)

Very much related to conversations happening here and here.

Testing

  • Please confirm npm run test-all ran successfully.

@vitalbone
Copy link
Contributor Author

So this is getting pretty complicated. The [email protected] dependency doesn't not work with any node engines lower than 6.x therefore we're unable to pass CI since we've backwards compatible to at least keystone v4.x.

The dependencies I can find that are using mime@^2.x are knox and superagent. I forked knox to change the mime dep from * to ^1.4.0 but then i discovered superagent is using [email protected] as well.

Am I meant to fork this dependency as well? Seems like not the best solution to the problem.

The bigger discussion is obviously backwards compatibility so looping in @JedWatson and @molomby so we can resolve this and move on.

@molomby
Copy link
Member

molomby commented Oct 8, 2017

@JedWatson I say we bump Keystone 4 to node ^6 for this next release. Maybe also open an issue along the lines of "Keystone 4 is not compatible with node version 5.x or earlier" to discuss and document the decision. We can always increase backward compatibility in future KS 4 releases. Lets just cut scope and get it out.

@JedWatson
Copy link
Member

@molomby agreed. The decision was made a long time ago and I think we need to move on. Let's open an issue separately for the update so it gets the proper visibility though, and not do it before the next beta release.

@JedWatson
Copy link
Member

@vitalbone can you update this PR to exclude those packages that have a requirement (or a dependency with a requirement) for node 6, and we'll update those after the next release?

Vito Belgiorno-Zegna added 2 commits October 11, 2017 09:08
- knox has been replaced with keystonejs/knox as the package is not being maintained anymore and has a `*` for `mime`
- [email protected] uses [email protected] (which is compatible with node <=6.x)
@vitalbone
Copy link
Contributor Author

@jed I have downgraded [email protected] as that is that last version to use [email protected]. We are now depending on a forked version of knox, one that uses [email protected]. At the moment we are pulling it in via a GitHub raw link, please let me know if you'd like to publish it to npm.

@vitalbone vitalbone requested a review from molomby October 31, 2017 00:41
@vitalbone
Copy link
Contributor Author

@molomby Probably worth checking the changes made to the Password tests, re: security fixes.

@vitalbone
Copy link
Contributor Author

Working locally and tested reasonably thoroughly via test-project. The build is breaking due to failing tests in the Admin [Group005 Item / Ux Test Item View] Test Suite. As per Jed's advice, going to merge in anyway.

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.

3 participants