Skip to content
This repository has been archived by the owner on Sep 8, 2021. It is now read-only.

explicitly export types #36

Merged
merged 3 commits into from
Dec 10, 2020
Merged

explicitly export types #36

merged 3 commits into from
Dec 10, 2020

Conversation

sndrs
Copy link
Member

@sndrs sndrs commented Dec 2, 2020

What does this change?

explicitly exports types

Why?

getting errors using --isolatedModules

image

Have we considered potential risks?

should be minimal because type exports became available in 3.8, and this package depends on ^3.8.3

@sndrs sndrs requested a review from JamieB-gu December 2, 2020 11:26
Copy link
Contributor

@JamieB-gu JamieB-gu left a comment

Choose a reason for hiding this comment

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

Uh oh, did I cause this problem with the refactoring I did for v1.0?

Changes look good, just a couple questions.

src/format.ts Outdated Show resolved Hide resolved
src/result.ts Outdated Show resolved Hide resolved
@sndrs sndrs marked this pull request as draft December 2, 2020 17:24
…xport

# Conflicts:
#	src/format.ts
#	src/index.ts
#	src/ophan.ts
#	src/option.ts
#	src/result.ts
#	src/role.ts
@sndrs sndrs marked this pull request as ready for review December 4, 2020 13:35
src/format.ts Show resolved Hide resolved
@sndrs sndrs requested a review from JamieB-gu December 4, 2020 13:41
@JamieB-gu
Copy link
Contributor

Is this a breaking change (v2.0) or a feature (v1.1)?

@sndrs
Copy link
Member Author

sndrs commented Dec 4, 2020

would it be a patch?

I don't think any exports have changed – will it affect anyone doing straight import { myType } from '@guardian/types'?

@JamieB-gu
Copy link
Contributor

JamieB-gu commented Dec 4, 2020

would it be a patch?

Is it a feature because you can now import some things as import type rather than import and get the isolated modules functionality?

will it affect anyone doing straight import { myType } from '@guardian/types'?

It seems to be interchangeable with normal imports? For example, we're already import typeing Theme in AR, even though it hasn't explicitly been a type export up until now. Presumably the inverse is also true?

@sndrs
Copy link
Member Author

sndrs commented Dec 4, 2020

For example, we're already import typeing Theme in AR, even though it hasn't explicitly been a type export up until now

does this mean it's not a new feature? :)

@JamieB-gu
Copy link
Contributor

JamieB-gu commented Dec 4, 2020

does this mean it's not a new feature? :)

Ah but that worked fine for us because we're not using isolated modules. Whereas my understanding was that you added this in order to get new functionality for projects that are using isolated modules?

Ultimately it probably doesn't matter that much 🤷‍♂️, I'm happy with either.

@sndrs
Copy link
Member Author

sndrs commented Dec 4, 2020

yeah that's a good point. I reckon that counts as a feature. how are releases done? do I need to update this PR?

@sndrs
Copy link
Member Author

sndrs commented Dec 4, 2020

also, did you see #38?

@JamieB-gu
Copy link
Contributor

JamieB-gu commented Dec 4, 2020

how are releases done? do I need to update this PR?

In the past, if there's just a single small change we've bumped in the change PR (e.g. #30). For larger changes, or multiple PRs going into a single change, I've done a separate PR just to bump the version (e.g. #35).

Following that we'll need a v1.1.0 tag to make its way onto main, a corresponding release, and a publish to the registry. I don't think it matters if you want npm/yarn to create the tag and then hang a GH release off it, or if you want to create the tag from the GH release and then hang the npm/publish off that tag.

It would be nice if we could automate this at some point, although we'd have to overcome the issues discussed in the CS Infra group relating to permissions. I did a bit of research and I think it would be easy enough to create a manually triggered GH action that creates a version bump PR. I think we could probably also do GH releases with GH actions.

@sndrs
Copy link
Member Author

sndrs commented Dec 7, 2020

would it be useful if I added np in a separate pr, so that you can npm run release or something similar?

@JamieB-gu
Copy link
Contributor

JamieB-gu commented Dec 8, 2020

would it be useful if I added np in a separate pr

Looks like it may cover the required steps that I outlined above. "Opens a prefilled GitHub Releases draft after publish" how does it do this bit? Does it require GH creds?

@sndrs
Copy link
Member Author

sndrs commented Dec 10, 2020

"Opens a prefilled GitHub Releases draft after publish" how does it do this bit? Does it require GH creds?

opens the page in your default browser with the fields pre-filled

@sndrs sndrs merged commit 40896dc into main Dec 10, 2020
@sndrs sndrs deleted the explicit-type-export branch December 10, 2020 15:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants