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

Specify frontend package dependency versions #951

Merged

Conversation

arescrimson
Copy link
Contributor

Contributor checklist


Description

This is for issue #872, where we are trying to specify frontend dependencies. This was done through just removing the carets and leaving a specific version number for each package. This has the effect of potentially resolving some issues where a released minor/major versions introduces new, unnecessary dependencies.

NOTE: This is still a W.I.P - @andrewtavis I've left it as a W.I.P just in case you wanted to hop on the branch and look at the initial changes for yourself, see if this is on the right track, etc.

W.I.P List:

Resolving unmet peer dependency issues - I assume this is because some of the packages were using versions that don't match the ones that are now specified, so I'll resolve those.

image
^ Looks like these warnings are mostly from one package, so hopefully straightforward fix.

Testing to see if the application will still run the same previously with the new packages - this I'm open to suggestions/guidance for :)

Related issue

Copy link
Contributor

github-actions bot commented Aug 26, 2024

Thank you for the pull request!

The activist team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Development rooms once you're in. Also consider joining our bi-weekly Saturday dev syncs. It'd be great to have you!

Maintainer checklist

  • The commit messages for the remote branch should be checked to make sure the contributor's email is set up correctly so that they receive credit for their contribution

    • The contributor's name and icon in remote commits should be the same as what appears in the PR
    • If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for git config user.email in their local activist repo
  • The TypeScript and formatting workflows within the PR checks do not indicate new errors in the files changed

  • The Playwright end to end and Zap penetration tests have been ran and are passing (if necessary)

  • The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

Copy link

netlify bot commented Aug 26, 2024

Deploy Preview for activist-org ready!

Name Link
🔨 Latest commit 65c21be
🔍 Latest deploy log https://app.netlify.com/sites/activist-org/deploys/66d23cd9a11bbb0008bde8aa
😎 Deploy Preview https://deploy-preview-951--activist-org.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@arescrimson
Copy link
Contributor Author

Looks like checks are failing because of @nuxt/eslint-config, which is the same one that's also producing those warnings. Honestly, from what I understand about eslint, I think we could maybe leave that one unspecified. Since it's just a linter, minor changes/releases hopefully just still be fine?

Either way, I'll start looking for the proper version that fulfills that missing peer dependencies - maybe that's also what's causing the checks to fail as well.

@andrewtavis andrewtavis self-requested a review August 26, 2024 19:58
@andrewtavis
Copy link
Member

Thanks for this, @arescrimson! I'll be able to help here once a few more PRs are in. I'm finalizing the i18n ones tonight and then will start on others 😊

@andrewtavis
Copy link
Member

Hey @arescrimson 👋 For the tests to see if things are working properly, for now sadly it's just "is stuff loading" as we don't have proper tests. I'll click around to make sure that things are progressing, and then we can make use of the browser tests we're building up eventually. Ultimately there's nothing to test against... Do you want to fix the dependency errors and merge conflicts and then we'll bring this in? Happy to help as needed!

@arescrimson
Copy link
Contributor Author

Yep! Sounds good @andrewtavis - I've been having some trouble because it looks like the errors are related to maybe how the eslint package is interacting with other packages. If I restore the eslint version to an unspecified one, the package still throws errors. Will continue working on this and hopefully will have more updates later 😄

arescrimson and others added 2 commits August 30, 2024 14:39
Resolved by adding loose versions to nuxt-eslint and eslint packages
@arescrimson
Copy link
Contributor Author

Hey @andrewtavis just some notes on this - I had to leave @nuxt-eslint and eslint as loose dependencies since they were the ones generating the errors. This is also because I think if they were tied to one specific version, so many different packages rely on the different versions of eslint that they kind of have to be loose or we'll get a bunch of missing/incorrect peer deps.

Let me know if it looks the same on your end, or if you want to change anything about this. So far, it looks like there are no warnings and that every package is working fine in their specified version, but just want to make sure 😄

@andrewtavis
Copy link
Member

This is great, @arescrimson! I'll check it out tomorrow and bring it in before doing the Dependabot Nuxt update from the other PR. Appreciate you looking into this! Makes sense that we still need some loose dependencies, and hopefully those won't cause us to need to update dependencies when we're doing frontend linting tests. I think likely we won't 😊

@arescrimson
Copy link
Contributor Author

Yep! I will say though @andrewtavis it was pretty straightforward to specify the versions, etc... but I think we all know how annoying the Node ecosystem can get sometimes when dealing with package dependencies, and I'm kind of thinking about how packages will interact going forward. For now, hopefully everything goes well, but in the future feel free to ping me if something starts breaking because that's something I'm a bit worried about 😅

In addition, hopefully this also resolves issue #871 - unless any new packages are added for now, hopefully shouldn't have any unmet peer dependencies. I just tested it out with a fresh lockfile and it looks like there's still one error when fetching packages, but this should be how it looks for a person installing the project:
image

Copy link
Member

@andrewtavis andrewtavis left a comment

Choose a reason for hiding this comment

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

Thanks so much for the efforts here, @arescrimson! Really great to have this figured out :) I'll play around a bit with the "trying to install in the same destination" error a bit so hopefully we can get a clean build 😊

@andrewtavis andrewtavis merged commit ddeea83 into activist-org:main Aug 31, 2024
6 checks passed
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