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.

Sign up for GitHub

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

Jump to bottom

fix: remove rimraf from security package #2594

Merged
merged 2 commits into from
Jul 31, 2020
Merged

fix: remove rimraf from security package #2594

merged 2 commits into from
Jul 31, 2020

Conversation

sirugh
Copy link
Contributor

@sirugh sirugh commented Jul 31, 2020 •
edited
Loading

Description

For some reason rimraf as a peer dependency in the upward security headers package was causing odd failures when trying to add a dependency to a package.

Moving rimraf to a devDependency, or just removing it entirely, fixed the issue.

Related Issue

Closes #ISSUE_NUMBER.

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Check out this PR.
  2. yarn install
  3. yarn workspace @magento/venia-concept add -D apollo-link-batch-http
  4. Verify that no error occurred when trying to add a dev dependency to venia-concept.

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have added tests to cover my changes, if necessary.
  • I have updated the documentation accordingly, if necessary.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Jul 31, 2020 •
edited
Loading

Fails
🚫

No linked issue found. Please link a relevant open issue by adding the text "closes #<issue_number>" or "closes JIRA-<issue_number>" in your PR.
🚫 A version label is required. A maintainer must add one.
Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against 53874f7

@devops-pwa-codebuild
Copy link
Collaborator

devops-pwa-codebuild commented Jul 31, 2020 •
edited
Loading

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2594.pwa-venia.com : LH Performance Expected 0.85 Actual 0.34, LH Accessibility Expected 1 Actual 0.97, LH Best Practices Expected 1 Actual 0.92, WPT Cache Expected 90 Actual 39.333333333333
https://pr-2594.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.34, LH Best Practices Expected 1 Actual 0.92
https://pr-2594.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.42, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92, WPT Cache Expected 65 Actual 52.333333333333

@sirugh sirugh changed the title Remove unneeded rimraf dependency Move rimraf to dev dependency in extension Jul 31, 2020
tjwiebell
tjwiebell reviewed Jul 31, 2020
View reviewed changes
packages/extensions/upward-security-headers/package.json Outdated
@@ -7,20 +7,22 @@
"description": "Add security headers to UPWARD",
"main": "intercept.js",
"scripts": {
"clean": " "
"clean": "rimraf ./node_modules"
Copy link
Contributor

Choose a reason for hiding this comment

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

clean seems to be more about removing dist in all other packages. Can we keep this line and just remove the rimraf dependency entirely?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with either approach, as long as the bug is fixed. Interested in QA outcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm - it removes node modules in the gql-queries package

"clean": "rimraf ./lib/magento-compatibility.js ./node_modules",

jimbo
jimbo previously approved these changes Jul 31, 2020
View reviewed changes
packages/extensions/upward-security-headers/package.json Outdated
@@ -7,20 +7,22 @@
"description": "Add security headers to UPWARD",
"main": "intercept.js",
"scripts": {
"clean": " "
"clean": "rimraf ./node_modules"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with either approach, as long as the bug is fixed. Interested in QA outcome.

@sirugh sirugh changed the title Move rimraf to dev dependency in extension fix: remove rimraf from security package Jul 31, 2020
@dpatil-magento dpatil-magento merged commit 32fcca5 into develop Jul 31, 2020
@dpatil-magento dpatil-magento deleted the rugh/fix-rimraf branch July 31, 2020 21:49
@sirugh sirugh restored the rugh/fix-rimraf branch April 26, 2021 16:29
@sirugh sirugh deleted the rugh/fix-rimraf branch April 26, 2021 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tjwiebell tjwiebell tjwiebell left review comments

@jimbo jimbo jimbo approved these changes

Assignees

@tjwiebell tjwiebell

@sirugh sirugh

@jimbo jimbo

Labels
pkg:extensions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sirugh @PWAStudioBot @devops-pwa-codebuild @tjwiebell @jimbo @dpatil-magento