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 @preconstruct/cli #5409

Merged
merged 1 commit into from
Oct 25, 2022
Merged

Update @preconstruct/cli #5409

merged 1 commit into from
Oct 25, 2022

Conversation

lukebennett88
Copy link
Collaborator

@lukebennett88 lukebennett88 commented Oct 21, 2022

Updates to the latest version of Preconstruct as the version we were using is quite old (version 2 came out two years ago).
This requires moving some important files around, so we want to be extra careful that this outputs the same entrypoints etc so that it's not a breaking change.

@changeset-bot
Copy link

changeset-bot bot commented Oct 21, 2022

🦋 Changeset detected

Latest commit: 456f8df

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

This PR includes changesets to release 1 package
Name Type
react-select Patch

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 21, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 456f8df:

Sandbox Source
react-codesandboxer-example Configuration

@lukebennett88 lukebennett88 force-pushed the luke/update-preconstruct branch 2 times, most recently from bae261a to 6d79c00 Compare October 21, 2022 02:16
@@ -0,0 +1,5 @@
---
'react-select': minor
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be patch since it's not changing the api?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not 100%, I was trying to play it safe call it a minor as some things that aren't necessarily part of the API contract might break after this gets merged.
e.g. if someone imports something directly from the dist folder.
This might even end up being a major change (in which case, I guess we can't merge it) as I just found this section in the docs:
https://react-select.com/typescript#custom-select-props

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This PR would change this snippet from the docs:

- declare module 'react-select/dist/declarations/src/Select' {
+ declare module 'react-select/dist/declarations/src/base' {
  export interface Props<
    Option,
    IsMulti extends boolean,
    Group extends GroupBase<Option>
  > {
    myCustomProp: string;
  }
}

It would also change:

-import AnimatedSelect from 'react-select/animated/dist/react-select.cjs.js.cjs'
+import AnimatedSelect from 'react-select/animated/dist/react-select-animated.cjs'

But people shouldn't be importing directly from the dist folder for this sort of thing anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need a decision on whether react-select/dist/* is considered part of the api, and if it is, then we should bump major version, otherwise if it isn't, it's only patch.

Alternatively, you could just leave the file structure the way it is (ie. don't move Select.tsx -> base/Select.tsx).
Although, i'm not sure how preconstruct would feel about the casing when it comes to Creatable and Async (because their entrypoints need to be the same name just lowercased)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated the PR to not move any files (and just add a bunch of folders and index files) so the PR is much smaller now.

The first example in my comment above should still work.

The second one I don't think I can fix, but I had a look and I don't think that would be a major problem.
The only files built in the extra entrypoints are different build targets for the same file, and since none of the files have moved, the contents of react-select/declarations/src will be the same (apart from the new index files).

As such, I've switched the changeset to be a patch.

@lukebennett88 lukebennett88 force-pushed the luke/update-preconstruct branch 3 times, most recently from 3bbd485 to a3a1891 Compare October 25, 2022 00:18
@lukebennett88 lukebennett88 force-pushed the luke/update-preconstruct branch from a3a1891 to 456f8df Compare October 25, 2022 00:39
@lukebennett88 lukebennett88 enabled auto-merge (squash) October 25, 2022 02:40
Copy link
Collaborator

@nderkim nderkim left a comment

Choose a reason for hiding this comment

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

Nice work! 🍉

@lukebennett88 lukebennett88 merged commit 9239893 into master Oct 25, 2022
@lukebennett88 lukebennett88 deleted the luke/update-preconstruct branch October 25, 2022 03:23
@github-actions github-actions bot mentioned this pull request Oct 25, 2022
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.

2 participants