solidjs / vite-plugin-solid

A simple integration to run solid-js with vite
440 stars 51 forks source link

add omitNestedClosingTags #121

Closed birkskyum closed 1 year ago

birkskyum commented 1 year ago

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 commented 1 year ago

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.

https://github.com/solidjs/vite-plugin-solid/blob/47957467a940a877afe746741545feab52eeb14d/src/index.ts#L193-L199 https://github.com/solidjs/vite-plugin-solid/blob/47957467a940a877afe746741545feab52eeb14d/src/index.ts#L384

birkskyum commented 1 year ago

Maybe even in here:

https://github.com/solidjs/solid-start/blob/2967fc2db3f0df826f061020231dbdafdfa0746b/packages/start/vite/plugin.d.ts#L13

birkskyum commented 1 year ago

made a new one here: https://github.com/solidjs/solid-start/pull/1086

edivados commented 1 year ago

~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 commented 1 year ago

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

edivados commented 1 year ago

@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

edivados commented 1 year ago

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. https://github.com/solidjs/vite-plugin-solid/blob/47957467a940a877afe746741545feab52eeb14d/src/index.ts#L404

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 commented 1 year ago

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.

https://github.com/solidjs/vite-plugin-solid/blob/47957467a940a877afe746741545feab52eeb14d/src/index.ts#L404

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 commented 1 year ago

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.