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

feat: introduce componentNameFormatter for react output target #235

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

markhughes
Copy link

@markhughes markhughes commented Mar 10, 2022

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally for affected output targets
  • Tests (npm test) were run locally and passed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • - [ ] Documentation content changes
    Other (please describe):

What is the current behavior?

N/A

What is the new behavior?

This feature introduces a config option for the react output target called componentNameFormatter which allows formatting the name of component names generated in react.

This is a very useful utility when you don't always want your name for your react package. For example, you may not want your react components to include the prefix, or you may want to add a prefix or suffix.

I have included an example in the README.md file.

Also resolves #122

Does this introduce a breaking change?

  • Yes
  • No

@markhughes markhughes force-pushed the feat/component-name-formatter branch from 25d663b to 7570e4d Compare March 10, 2022 01:23
@markhughes
Copy link
Author

HI @rwaskiewicz could I ping this for a review please?

@sean-perkins
Copy link
Collaborator

Hello @markhughes thanks for the PR!

At this time we aren't prepared to review/merge this feature request, for a few reasons:

  1. Formatting features such as this, should apply to all supported output targets. Patching a feature for one output target without adding support to others, causes inconsistencies in usage patterns as well as diverges the codebase further.
  2. This feature can/does solve the need for re-formatting the generated output, but has no safe guards/validation to prevent developers from creating an invalid class name.
  3. While the problem can be addressed from the output targets, we would like to explore if this problem can be better solved from the Stencil compiler. We have similar requests for the raw web components, to have the ability to rename tags/classes/versioning.

The next steps for this PR would be:

  • Open an issue on this repository to help gather and gauge the requirements of this feature.
  • Stencil team would need to schedule time to investigate the aspects of this feature within the context of the compiler (this is not currently on our roadmap or a commitment the team is making).
    • If the Stencil team can validate and solve the feature, this PR can be closed and the associated issue can be closed.
    • If the Stencil team identifies that this problem is best solved from the output targets, we would need to rework this PR based on my top two points and reviewed by the team.

I realize this may not be ideal to the problems you are trying to solve and may also not align with your timeline. If that is the case, I would encourage you to use a forked version of this repository in the interim, so that you are able to solve your challenges.

I would also encourage you to avoid directly pinging team members. While some individuals may appreciate the reminder, others may also find it to be interrupting or distracting. As a best practice, I tend not to tag individuals unless they are already involved in the conversation and I am replying to them.

Let me know if you have any questions or need guidance on how to use a forked version.

@markhughes
Copy link
Author

markhughes commented Mar 18, 2022

Hi @sean-perkins thanks for the response.

  1. & 3. I disagree. A React component naming convention compared to web component naming conventions, and other frameworks, are very different. For example, you may name something ionic-button in web components but react libraries tend to pick naming conventions of either Button or IonicButton - React is unopinionated and we should respect that. This naming convention doesn't make sense in other libraries.

  2. Great point! Feedback applied. Note that react devs recommend writing functional components, not class components.

Moving on,

Open an issue on this repository to help gather and gauge the requirements of this feature.

This has already been reported over a year ago. I referenced the ticket (#122) in my original opening message. I was planning on attending a bunch of simple issues neglected in this project.

I realize this may not be ideal to the problems you are trying to solve and may also not align with your timeline. If that is the case, I would encourage you to use a forked version of this repository in the interim, so that you are able to solve your challenges.

I am just trying to un-neglect this projects issues.

I would also encourage you to avoid directly pinging team members. While some individuals may appreciate the reminder, others may also find it to be interrupting or distracting. As a best practice, I tend not to tag individuals unless they are already involved in the conversation and I am replying to them.

This projects issues and PR's are heavily neglected, some are up to years of people trying to get help or fix issues, and I could only see you as the most recent contributor. I apologise that ended up you being pinged but I was literally told to ping a dev based on the previous response time (which is usually never) in this project.

With this in mind I now have two concerns moving forward:

  1. why should my company and others even consider continuing along with an enterprise subscription?
  2. this project is heavily neglected in terms of issue responses and PR to the point the team has to be pinged to respond (and gets upset when a ping is done), is this a structural problem with Ionic? This is a business risk for anyone interested in Ionic.

@sean-perkins
Copy link
Collaborator

@markhughes thanks for your thoughts and feedback!

I agree completely to the convention of tag names within React. The advantage of allowing selectors and component classes names in Stencil to be formatted from the compiler before output targets, is that the output targets responsibility simply becomes formatting ionic-button to IonicButton. It also allows each output target to internally benefit from the feature out-of-the-box, without having to replicate the API and implementation for each output target.

I also agree that attention and maintenance on this project as a whole has been unclear and inconsistent. The purpose of many of the output targets has historically been focused on the needs of the Ionic Framework project. Realizing that the output targets were beneficial to the whole Stencil community, they were open sourced so that others may use them, without an alignment or plan on maintenance and further development.

The team has started to work on that alignment and plan. Both the Framework dev team and the Stencil dev team now co-owns this repository. Stencil team's focus is on maintaining the compiler functionality against how the output targets are consumed and the Framework team is focused on the features actively used inside of Ionic Framework. I additionally have taken the responsibility of both triaging new issues (working on getting to existing issues) and backfilling process around: codeowners, contributing guidelines, Github actions for PR builds and release management, etc. It will take some time to bring this project up to speed and conventions. This project also requires a huge lift in terms of documentation and examples. If we can collaborate at the Github Issue level first, I'd be welcoming to contributions after we align on approach/scope. The unfortunate side effect at going PR first outside of a proof of concept, is that it already commits your time without certainty that the work will be proceeded with.

I personally do not have issues with being pinged, as long as the expectation is not always an immediate response. I was just relaying feedback I have received in the past with different team members and trying to be respectful to their existing commitments.

Addressing your concerns, here are my thoughts:

  1. Enterprise support addresses many different aspects of the development process. I am unsure what commitment your support contains, but I believe many include a direct line of contact with members of our team. Using that process, we can collaborate a lot closer/realtime to identify and address issues. For instance, in terms of enterprise support on Framework, we deliberately pull in enterprise support issues into our sprint during planning, in addition to other feature development/priority bugs. I've also addressed several issues even this past week within a 24 hour turn-around of the issue being communicated to the internal teams. It is likely dependent on what types of support you are needing and I am unsure if it overlaps/includes PR reviews/contributions.
  2. The project's maintenance issues are being addressed, but also requires time to resolve. All recent Github issues have a response. Waiting 10 days for a PR that introduces a feature to be reviewed/replied to is not necessarily a structural issue, but is a process we can be more explicit with. If customers with enterprise support need to get in contact with our team, they are instructed to use our helpdesk to expedite that communication. From my experience as someone that helps address those issues, the team responsible for handling that communication is very attentive and also extremely technical.

I believe next steps from this lengthy thread is:

  1. Pulling the conversation of how to rewrite tags/classes/etc. up to the issue level and allow the community and yourself to provide feedback if that solution has any unforeseen downsides.
  2. If you are an existing enterprise customer, areas of this likely do fall in your support agreement. If so, please open a ticket with our helpdesk with a brief summary of your issues/pain point/blockers (you can link this issue if you'd like for reference) and we can resource time to resolve it.
  3. In the interim of this project being brought to standards, I can open a pinned ticket or discussion that actively reflects the state of the repo, the teams responsible, what areas we are supporting with this repo, etc.

I will take care of 1 and 3 on Monday. I hope you have a good weekend!

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.

feat: Ability to rename/format generated component names
2 participants