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

Fix build issues #72

Merged
merged 4 commits into from
Jun 15, 2023
Merged

Fix build issues #72

merged 4 commits into from
Jun 15, 2023

Conversation

felipecadavid
Copy link
Contributor

@felipecadavid felipecadavid commented Apr 20, 2023

Description of changes

This PR fixes some Parcel build issues found including yarn build and yarn dev getting stuck.

Checklist

Before merging to main:

  • Tests
    • N/A
  • Manually tested in React apps
    • N/A
  • Release notes
    • This relates only to the build system and can be omitted.
  • Approved

Release notes

N/A

@vercel
Copy link

vercel bot commented Apr 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
ui-kit ⬜️ Ignored (Inspect) Visit Preview Jun 15, 2023 4:11pm

Copy link
Contributor

@jonatassales jonatassales left a comment

Choose a reason for hiding this comment

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

Looking good. I just don't know why you removed the ^ prefix in the sem versions in package.json

package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@jonatassales jonatassales left a comment

Choose a reason for hiding this comment

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

I noticed you're sticking on a specific Parcel version. Why is that?

Copy link
Contributor

@markandrus markandrus left a comment

Choose a reason for hiding this comment

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

@felipecadavid I took a look, and had a few questions.

One thing I am thinking: using Yarn PnP here might actually be good, because it will force us to be strict with dependencies.

Comment on lines +32 to +33
"@parcel/packager-ts": "^2.8.2",
"@parcel/transformer-typescript-types": "^2.8.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change here? I mean, it's SemVer-compatible, but why change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found an issue in parcel version 2.8.3 that was affecting the build time so I rolled it back to 2.8.2
The latest version works correctly though, so I think we can keep like this and keep the ^ for @parcel/transformer-typescript-types as well

packages/core/plugins/package.json Show resolved Hide resolved
packages/core/graphql/package.json Show resolved Hide resolved
Copy link
Contributor

@markandrus markandrus left a comment

Choose a reason for hiding this comment

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

LGTM — however, let's think tomorrow about how to document the changes here (e.g., in a CHANGELOG.md, or in GitHub Release notes). We can consider this a "bug fix" I think, since there was at least one missing dependency.

@markandrus markandrus merged commit 5670ded into main Jun 15, 2023
3 checks passed
@markandrus markandrus deleted the refactor/fix-build-issues branch June 16, 2023 08:15
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.

None yet

3 participants