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: custom component name should be constrained explicitely - WF-52 #550

Conversation

FabienArcellier
Copy link
Collaborator

fix #517


image

@FabienArcellier FabienArcellier self-assigned this Sep 2, 2024
@FabienArcellier FabienArcellier added the bug Something isn't working label Sep 2, 2024
@FabienArcellier FabienArcellier changed the title fix: custom component name should be constrained explicitely fix: custom component name should be constrained explicitely - WF-52 Sep 2, 2024
src/ui/src/custom_components/core.ts Outdated Show resolved Hide resolved
Comment on lines 26 to 38
const is_valid_key = regex.test(key);
if (!is_valid_key) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In JS, we prefer camlCase like isValidKey.

Suggested change
const is_valid_key = regex.test(key);
if (!is_valid_key) {
const isValidKey = regex.test(key);
if (!isValidKey) {

Comment on lines 28 to 30
throw Error(
`custom component key '${key}' is not valid. custom component should be declared with only alphanumeric lowercase and _.`,
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question, do we really want to stop the application here ? Do you think console.warn could be enough ?

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 asked myself the question. I decided to raise an explicit error. A simple warning might be enough. I will adapt this message in this case.

*
* @param customComponents list of components in key-value form
*/
export function declareCustomComponents(
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 the function name could be more meaningfull. Maybe checkCustomComponents

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case I want to call the function like this declareCustomComponents. It seems more logical when you read the calling code :

export default declareCustomComponents({
	bubblemessage: BubbleMessage,
	bubbleMessageadvanced: BubbleMessageAdvanced,
});

@FabienArcellier FabienArcellier force-pushed the 517-custom-component-name-should-be-constrained-explicitely branch 3 times, most recently from d426e9e to d8ffa7f Compare September 2, 2024 15:25
@FabienArcellier FabienArcellier marked this pull request as ready for review September 2, 2024 15:27
@ramedina86
Copy link
Collaborator

@madeindjs thanks for taking a look at this

@FabienArcellier I think we should check during load, it'll be more straightforward and reliable. Also, let's try to keep core as lean as we can.

We can intercept the keys here:

importCustomComponentTemplate in loadExtensions.ts (for loading via transpiled extension)
if (WRITER_LIVE_CCT === "yes") {... in templateMap.ts (for loading live while developing)

@FabienArcellier FabienArcellier force-pushed the 517-custom-component-name-should-be-constrained-explicitely branch from d8ffa7f to 8990e9f Compare September 16, 2024 10:13
@FabienArcellier FabienArcellier force-pushed the 517-custom-component-name-should-be-constrained-explicitely branch from 8990e9f to ef54bcc Compare September 16, 2024 10:16
@FabienArcellier
Copy link
Collaborator Author

FabienArcellier commented Sep 16, 2024

@madeindjs thanks for taking a look at this

@FabienArcellier I think we should check during load, it'll be more straightforward and reliable. Also, let's try to keep core as lean as we can.

We can intercept the keys here:

importCustomComponentTemplate in loadExtensions.ts (for loading via transpiled extension) if (WRITER_LIVE_CCT === "yes") {... in templateMap.ts (for loading live while developing)

I have implemented this approach. I continue looking for a better way to raise the error during the build or using a dedicated npm command.

The problem only appears currently in runtime when you open the web application. It does not feel great.

@FabienArcellier
Copy link
Collaborator Author

I have implemented a command npm run custom.check that verify this rule. It will allow to check the status without running the entire app and allow a person to implement a start of continuous integration pipeline when building custom components. I add the info in the documentation.

image

* feat: add a command npm run ui:custom.check
@FabienArcellier FabienArcellier force-pushed the 517-custom-component-name-should-be-constrained-explicitely branch from 38fa575 to 7f89714 Compare September 16, 2024 12:30
@ramedina86 ramedina86 merged commit 720ad31 into writer:dev Sep 20, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

custom component name should be constrained explicitely
3 participants