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

add omitNestedClosingTags #121

Closed
wants to merge 2 commits into from

Conversation

birkskyum
Copy link
Contributor

@birkskyum birkskyum commented Oct 12, 2023

Added here https://github.com/solidjs/solid/blob/main/CHANGELOG.md#smaller-templates

This change allow type hints to show up when configuring the vite plugin.

@edivados
Copy link

edivados commented Oct 12, 2023

Doesn't this belong under the solid prop so it ends up in babel-plugin-jsx-dom-expressions or maybe added to solidOptions? I don't see how the flag would be passed down otherwise.

/**
* Pass any additional [babel-plugin-jsx-dom-expressions](https://github.com/ryansolid/dom-expressions/tree/main/packages/babel-plugin-jsx-dom-expressions#plugin-options).
* They will be merged with the defaults sets by [babel-preset-solid](https://github.com/solidjs/solid/blob/main/packages/babel-preset-solid/index.js#L8-L25).
*
* @default {}
*/
solid: {

let solidOptions: { generate: 'ssr' | 'dom'; hydratable: boolean };

@birkskyum
Copy link
Contributor Author

@birkskyum birkskyum closed this Oct 12, 2023
@birkskyum
Copy link
Contributor Author

made a new one here:
solidjs/solid-start#1086

@edivados
Copy link

edivados commented Oct 12, 2023

Well yes additionaly it should be defined in start. I take this back the solid prop in start probably points to this anyway and does not need it. I was doing this in start to pass it down. But i think the prop is actually missing here. If you use it without start...

plugins: [solid({
    solid: {
      omitNestedClosingTags: false
    }
  })]

@birkskyum birkskyum deleted the omitNestedClosingTags branch October 12, 2023 14:36
@birkskyum
Copy link
Contributor Author

@edivados , can you make a PR to fix it for the non-start use case?

@edivados
Copy link

@edivados , can you make a PR to fix it for the non-start use case?

I can but I don't think there was anything wrong with your PR here.

Start's Option type just points to here anyway:
https://github.com/solidjs/solid-start/blob/2967fc2db3f0df826f061020231dbdafdfa0746b/packages/start/vite/plugin.d.ts#L8

@birkskyum birkskyum mentioned this pull request Oct 12, 2023
@edivados
Copy link

edivados commented Oct 12, 2023

Commenting here because I don't want to pollute your new PR with more comments.
@birkskyum I was questioning if this PR is maybe incomplete. The prop is now defined but has to be passed to babel-plugin-jsx-dom-expressions.

If left like is, the value has to be taken and added to solidOptions so it's in presets.

presets: [[solid, { ...solidOptions, ...(options.solid || {}) }]],

But if its defined under the solid prop it gets automatically added to presets. See the options.solid spread. That's what I think at least and I don't know what the right call is here.

Regarding the type hint, the moment it's added here it should show up in solid-start too as start extends this type here.

@birkskyum
Copy link
Contributor Author

birkskyum commented Oct 12, 2023

Commenting here because I don't want to pollute your new PR with more comments. @birkskyum I was questioning if this PR is maybe incomplete. The prop is now defined but has to be passed to babel-plugin-jsx-dom-expressions.

If left like is, the value has to be taken and added to solidOptions so it's in presets.

presets: [[solid, { ...solidOptions, ...(options.solid || {}) }]],

But if its defined under the solid prop it gets automatically added to presets. See the options.solid spread. That's what I think at least and I don't know what the right call is here.

Regarding the type hint, the moment it's added here it should show up in solid-start too as start extends this type here.

I've moved it to options.solid now. Do you think that'll work?

@edivados
Copy link

It should I tested it within a solid-start project but you never know... I am intereseted to see what the maintainers input is on this.

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