8000 Release 0.14.24 by FlorianRappl · Pull Request #513 · smapiot/piral · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Release 0.14.24 #513

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 22 commits into from
May 13, 2022
Merged

Release 0.14.24 #513

merged 22 commits into from
May 13, 2022

Conversation

FlorianRappl
Copy link
Contributor

New Pull Request

For more information, see the CONTRIBUTING guide.

Prerequisites

Please make sure you can check the following boxes:

  • I have read the CONTRIBUTING document
  • My code follows the code style of this project
  • All new and existing tests passed

Type(s) of Changes

Contribution Type

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue, please reference the issue id)
  • New feature (non-breaking change which adds functionality, make sure to open an associated issue first)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • I have updated the documentation accordingly
  • I have added tests to cover my changes

Description

Fixes an issue with Node 17 with removed the support for the trailing slash notation on exports (only star exports work now).

Also added two props that have been demanded for a while on ExtensionSlot. Now the behavior of empty can be controlled a bit. Beforehand it always called empty and then render, now render may be discarded in case of a desired different envelop for empty. The order prop can be helpful, too, when render seems to powerful / too much work.

Remarks

Potentially we'll also add support for rush as monorepo driver. Not sure yet.

@FlorianRappl FlorianRappl added the release A new release label May 5, 2022
manuelroemer
manuelroemer approved these changes May 5, 2022
View reviewed changes
Copy link
Member
@manuelroemer manuelroemer left a comment

Choose a reason for hiding this comment

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

Looks great! 🚀

The only thing I'd throw into the room is that I thought of perhaps renaming noEmptyRender to something like skipCustomRenderWhenEmpty? When I first looked at the test case above, noEmptyRender, without looking at the docs, made me think that nothing would be rendered when the slot is empty, i.e. that it might allow to conditionally hide the custom empty state in certain conditions. But maybe that's just me being weird... 😄

@FlorianRappl
Copy link
Contributor Author

And I thought noEmptyRender is already long and clunky - but skipCustomRenderWhenEmpty brings it to the next level. I think the main issue is that the prop is called render - which is the same as the name of the process in general.

How about emptySkipsRender? Essentially thats also a description whats happening. Instead of rendering (or callling the render prop) we just take the output of empty().

@manuelroemer
Copy link
Member
manuelroemer commented May 5, 2022 •
edited
Loading

but skipCustomRenderWhenEmpty brings it to the next level.

Hehe, point taken - though I don't necessarily find long names bad if they make things clearer (at least with IntelliSense...).
But emptySkipsRender sounds nice! We can also just stick with noEmptyRender honestly - just wanted to throw the thought into the room. It's not that that name is a problem.

I think the main issue is that the prop is called render - which is the same as the name of the process in general.

That's why I thought about the Custom in the name above: To give a hint that this refers to the provided render function, not the rendering process in general. But I think that once you do know what the props actually do, render is a good name for it.

manuelroemer
manuelroemer approved these changes May 6, 2022
View reviewed changes
Copy link
Member
@manuelroemer manuelroemer left a comment

Choose a reason for hiding this comment

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

Awesome stuff, great to have a PNPM workaround mentioned already! 🚀

This was linked to issues May 9, 2022
Improve Support for pnpm #514
Closed
Add Explicit Support for Rush #515
Closed
@FlorianRappl FlorianRappl added this to the 0.14.24 milestone May 9, 2022
@FlorianRappl FlorianRappl merged commit 467f56b into main May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manuelroemer manuelroemer manuelroemer approved these changes

@Majd-sufian Majd-sufian Awaiting requested review from Majd-sufian

Assignees
No one assigned
Labels
release A new release
Projects
None yet
Milestone
0.14.24
Development

Successfully merging this pull request may close these issues.

Add Explicit Support for Rush Improve Support for pnpm
2 participants
@FlorianRappl @manuelroemer
0