Skip to content

Make @parcel/watcher optional #9506

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

Merged
merged 1 commit into from
Jul 7, 2023
Merged

Conversation

valkum
Copy link
Contributor

@valkum valkum commented Jun 16, 2023

🚨 IMPORTANT: Please do not create a Pull Request without creating an issue first.

Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of the pull request.

Description

This is a new attempt to fix #9320.
@parcel/watcher needs a whole lot of build dependencies on the system. Normally, it will just pull prebuilt binaries for your system. But currently there are no prebuilt binaries for linux on arm64: parcel-bundler/watcher#130
In those environments, it tries to build it from source which means it needs python etc.
This makes @parcel/watcher optional, which should prevent these issues. The user is free to choose if they want to include @parcel/watcher as a dev dependency or take additional measures such as marking it optional (to ignore build errors)

The tests still run and @parcel/watcher should be pulled in via the devDependencies for the tests.
I assumed this would be a breaking change and included a major changeset and a migration guide.

Related #9320

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • I have followed the CONTRIBUTING doc and the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Further comments

This is the proposed way laid out in the yarn docs. The previous PR just moved @parcel/watcher into optionalDependencies. Yarn says using an optional peer dependency might be better.

@changeset-bot
Copy link

changeset-bot bot commented Jun 16, 2023

🦋 Changeset detected

Latest commit: f105677

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@graphql-codegen/cli Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@n1ru4l n1ru4l requested review from saihaj and n1ru4l June 19, 2023 07:33
Copy link
Collaborator

@n1ru4l n1ru4l left a comment

Choose a reason for hiding this comment

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

Thank you so much for tackling this properly! 🎉
Approved from my side.

@n1ru4l n1ru4l requested review from dotansimha and beerose June 19, 2023 07:36
@valkum valkum force-pushed the optional-watcher branch from 405fb6a to f105677 Compare June 19, 2023 10:53
@saihaj saihaj merged commit dd9c7e1 into dotansimha:master Jul 7, 2023
saihaj pushed a commit that referenced this pull request Oct 12, 2023
* Surface error occurring during import of @parcel/watcher

Follow up from #9506

* Add changeset

* Improve message and docs

* due to

* prettier
@glasser
Copy link

glasser commented May 23, 2025

In what way is this a breaking change for a user?

@valkum
Copy link
Contributor Author

valkum commented May 23, 2025

As in, previously it was installed as a dependency and for a user to still make use of it, they need to actively add it to their package.json to install it now.

@glasser
Copy link

glasser commented May 24, 2025

OK — how would I know if my configuration made use of this feature? Is there a particular thing to look for in my config file that would suggest that this is a breaking change for me?

@valkum
Copy link
Contributor Author

valkum commented May 24, 2025

Did your workflow depend on graphql-codegen --watch? If not this is not affecting you. If it was affecting you, you would have seen the following in your terminal:

Failed to import @parcel/watcher due to the following error (to use watch mode, install https://www.npmjs.com/package/@parcel/watcher):

@glasser
Copy link

glasser commented May 27, 2025

Great — might help to update the release notes to say something explicit like "The graphql-codgen --watch command now requires you to install @parcel/watcher yourself" so that people who don't use that flag don't have to worry about whether the upgrade will break them!

@valkum
Copy link
Contributor Author

valkum commented May 27, 2025

This was merged and released 2 years ago. I guess someone with write access could amend the release notes for https://github.com/dotansimha/graphql-code-generator/releases/tag/release-1690276829878.

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.

Make "@parcel/watcher" dependency optional
5 participants