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

feat: move "ipx" into optional dependencies which can be skipped #380

Closed
wants to merge 2 commits into from
Closed

feat: move "ipx" into optional dependencies which can be skipped #380

wants to merge 2 commits into from

Conversation

PatrickHeneise
Copy link
Contributor

Move ipx into optionalDependencies. These can be skipped with --no-optional (npm) or --ignore-optional (yarn) when external image optimization APIs are used (in production systems). sharp has a few install requirements and binaries which can be tricky in some situations.

Closes #379

src/ipx.ts Outdated Show resolved Hide resolved
@PatrickHeneise PatrickHeneise requested a review from pi0 August 12, 2021 11:17
@PatrickHeneise
Copy link
Contributor Author

@pi0 would appreciate a review on this. Running into build issues again on M1.

@pi0
Copy link
Member

pi0 commented Sep 5, 2021

Hi @PatrickHeneise sorry for the delay and thanks again for helping on this. It is now merged via #403 and will be released shortly as a fix :)

BTW some notes:

  • Please when making a pull-request, ensure "Allow edit from maintainers" so that we can directly push cleanup before merge
  • It is usually better to avoid doing unrelated changes in a PR (in this case it was recreation of yarn.lock)
  • If ipx is failing on M1, your Node.js installation is probably old. Which I would recommend check docs to troubleshoot this issue. Even if not using ipx/sharp, it can probably be good to fix root cause on your local machine :)

@pi0 pi0 closed this Sep 5, 2021
@PatrickHeneise
Copy link
Contributor Author

PatrickHeneise commented Sep 6, 2021

Thanks @pi0. I didn't know about the "allow editing for maintainers" option, will make sure that's enabled next time. I'm running Node 16. The problems are cross-arch build issues when Docker is involved.

I'm not familiar with yarn, but when I move/remove something in the dependencies (which happened here), the lock file needs to be updated, as otherwise it's still installed on production systems with npm ci. Is that not the case with yarn?

@pi0
Copy link
Member

pi0 commented Sep 6, 2021

Indeed when updating dependencies, yarn lock needs to be updated :)

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.

Skipping sharp dependency?
2 participants