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 graphql dependencies #224

Closed
wants to merge 3 commits into from
Closed

Conversation

leemyongpakvn
Copy link
Contributor

@leemyongpakvn leemyongpakvn commented May 31, 2023

Questions Answers
Description? Bump 4 dependencies :
graphql from 14.0.2 to 14.5.8
graphql-tag from 2.10.3 to 2.12.1
graphql-tools from 4.0.7 to 5.0.0
graphql-type-json from 0.3.1 to 0.3.2
August 12: Rebase after v3.0.1 release, lint fix and rebuild asset
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #{issue URL here}, Fixes #{another issue URL here}
Sponsor company Your company or customer's name goes here (if applicable).
How to test? Build Release pass. Module works as usual after PR applied (don't forget to rebuild asset and clear browser cache before test ;).

@M0rgan01
Copy link
Contributor

M0rgan01 commented Jun 1, 2023

Hello @leemyongpakvn :) package-lock.json has not been updated ?

@leemyongpakvn
Copy link
Contributor Author

@M0rgan01 I think package-lock.json and 14 assets files will be renewed by Build & Release workflow automatically. QA team will be happier with simple Files changed ;)

@M0rgan01
Copy link
Contributor

M0rgan01 commented Jun 1, 2023

The CI will not update your package-lock on the repository. It also seems that there are already problems with the lock file on the dev branch.

A command npm audit fails because of differences between package.json and package-lock.json.

To avoid this, you should replace npm install with an npm ci in the workflow 😃

jobs:
  deploy:
    name: build dependencies & create artifact
    runs-on: ubuntu-latest
    steps:
      - name: Checkout
        uses: actions/checkout@v3

      - name: Clone PrestaShop for core dependencies
        run: |
          git clone --depth=50 https://github.com/PrestaShop/PrestaShop.git ~/PrestaShop
          rm -rf ~/PrestaShop/modules/blockwishlist
          cd ..
          mv blockwishlist ~/PrestaShop/modules
          ln -s ~/PrestaShop/modules/blockwishlist
          cd blockwishlist

      - name: Install Node.js
        uses: actions/setup-node@v3
        with:
          node-version: 14.x

      - run: npm ci

@leemyongpakvn
Copy link
Contributor Author

@M0rgan01 package.json and package-lock.json will be updated together by this PR. You can create another PR to replace npm install by npm ci in workflow (I saw someone did it somewhere in this project but don't remember exactly who and when :).

@leemyongpakvn
Copy link
Contributor Author

@M0rgan01 Found it now PrestaShop/hummingbird#480

@M0rgan01
Copy link
Contributor

M0rgan01 commented Jun 1, 2023

Thanks @leemyongpakvn , I'll do it :)

Copy link

@tleon tleon left a comment

Choose a reason for hiding this comment

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

Hello @leemyongpakvn , the description of you PR is lacking some informations (The tickets numbers and the sponsor company filed). If you could complete it, it would greatly help our QA team.
Also IMO, I think it could be a good idea to update the modules in differents PR if possible ( I know it's not always possible since some packages have requierments ).

For this time you can let it as is ;)

Good work anyway and thanks for the contribution.

@leemyongpakvn leemyongpakvn marked this pull request as draft June 2, 2023 01:08
@leemyongpakvn
Copy link
Contributor Author

Waiting for the result of the related discussion

@leemyongpakvn leemyongpakvn marked this pull request as ready for review August 12, 2023 10:42
@leemyongpakva
Copy link

Close in favor of #250

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

Successfully merging this pull request may close these issues.

4 participants