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

Extensions option added #24

Merged
merged 5 commits into from
Jan 3, 2022
Merged

Extensions option added #24

merged 5 commits into from
Jan 3, 2022

Conversation

high1
Copy link
Contributor

@high1 high1 commented Jan 2, 2022

  • added extensions option, which should contain list of additional
    extensions that should be also processed by vite-plugin-solid

- added extensions option, which should contain list of additional
extensions that should be also processed by vite-plugin-solid
Copy link
Member

@amoutonbrady amoutonbrady left a comment

Choose a reason for hiding this comment

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

Did a quick pass, ping me if you have any questions :)

src/index.ts Outdated
Comment on lines 324 to 320
if (id.includes('tsx')) {
opts.presets.push([ts, options.typescript || {}]);
}
Copy link
Member

Choose a reason for hiding this comment

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

@high1 Can we keep this format? I prefer things to be explicit rather than one liners. It eases the readability in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've did this because
if (!/.[jt]sx/.test(id)) return null;
line 296

src/index.ts Outdated
Comment on lines 304 to 306
if (!(/\.[jt]sx/.test(id) || options.extensions?.includes(getExtension(id).split('?')[0])))
return null;

Copy link
Member

Choose a reason for hiding this comment

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

@high1 I'd prefer to break that into two different steps to improve readability.
Something like that:

Suggested change
if (!(/\.[jt]sx/.test(id) || options.extensions?.includes(getExtension(id).split('?')[0])))
return null;
if (options.extensions) {
const extension = getExtension(id);
const [extensionWithoutMdFlag] = extension.split('?');
if (options.extensions.includes(extensionWithoutMdFlag)) return null
}
if (!(/\.[jt]sx/.test(id)) return null;

Copy link
Contributor Author

@high1 high1 Jan 2, 2022

Choose a reason for hiding this comment

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

Sure, I have some kind of passion for one liners, but those can be hard to read...

Copy link
Member

Choose a reason for hiding this comment

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

Totally get it, I too have this thing, it's really a JS thing haha
The issue here is that I want anyone to be able to come in and be able to contribute without having to worry about code structure.
If we had another check to do within that if, that person would have to add { }

@nksaraf
Copy link
Member

nksaraf commented Jan 2, 2022

As per discussions, I also think its a good idea to have the options passed with the extensions be an object with config for the compiler..
eg.

	solidPlugin({ 
      extensions: [".md", [".mdx", { typescript: true }]] 
    })

This will allow us to add more options if we need them (and considering the compiler approach, its probably going to require configuration for like custom renderers, etc).

high1 and others added 4 commits January 3, 2022 01:22
- made everything more explicit, as requested
- added optional typescript opt-in behavior per extension, like
['.solid', { typescript: true }]
- check if options.extensions is defined before calling find
@amoutonbrady amoutonbrady merged commit 0416a1a into solidjs:master Jan 3, 2022
amoutonbrady added a commit that referenced this pull request Jan 26, 2022
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.

3 participants