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

Parametric compile options #12415

Open
Rich-Harris opened this issue Jul 11, 2024 · 13 comments
Open

Parametric compile options #12415

Rich-Harris opened this issue Jul 11, 2024 · 13 comments
Assignees

Comments

@Rich-Harris
Copy link
Member

Describe the problem

It's often useful to have different compiler options for different files. For example, you might want to enforce that all your first-party components are in runes mode, while continuing to use non-runes components in node_modules, or you might want to programmatically define custom element names rather than manually specifying them with <svelte:options> in each component.

Today, this is possible using dynamicCompileOptions in vite-plugin-svelte, but it has drawbacks:

  • you have to be using vite-plugin-svelte
  • it's an extra thing you have to learn; it feels a bit weird
  • things like VSCode and svelte-check can't take advantage of it

Describe the proposed solution

For the options where it makes sense, allow functions to be specified instead of values:

// svelte.config.js
export default {
  compilerOptions: {
    css: ({ filename }) => filename.includes('/og-cards/') ? 'injected' : 'external',
    runes: ({ filename }) => filename.includes('/node_modules/') ? undefined : true
  }
};

Importance

nice to have

@dominikg
Copy link
Member

dominikg commented Jul 12, 2024

to avoid multiple options turning into possibly function, what about having compile(r)Options itself be a function. The tricky bit is filename here as thats currently part of options.

export function compile(source, options) {

if it was compile(soure|{source,filename},options|({source,filename})=>options) it could work, but would require updates to all tooling that calls compile to pass filename along outside of options

@dominikg
Copy link
Member

dominikg commented Jul 12, 2024

or introduce a new dynamicCompile wrapper that calls compile after resolving the options to avoid the overloaded signature of compile

@dummdidumm
Copy link
Member

We discussed both these things and came to the conclusion that function per option is the best solution:

  • most of these options are unrelated to each other. That means that you likely want some hardcoded to be always the same, some to be dependent on condition X and others on condition Y. If you would have compilerOptions: ({ filename }) => .. then you would have a cumbersome-to-write-and-maintain if else chain in there. Much easier to have a function per option in this case
  • introducing a wrapper means that the compiler is still agnostic to this, which feels cleaner, but then each tooling has to implement that wrapper, leading to duplicate code and we might as well move it into core

@Rich-Harris
Copy link
Member Author

Another possibility is that we don't change the svelte.compile API at all, and this is just a svelte.config.js thing, since that's what gives equal footing to Vite, VSCode, svelte-check and all the rest of it. And that way — since you don't specify filename in your svelte.config.js — we can have a single function for everything, which would definitely make things more compact:

// svelte.config.js
export default {
  compilerOptions: ({ filename }) => ({
    css: filename.includes('/og-cards/') ? 'injected' : 'external',
    runes: filename.includes('/node_modules/') ? undefined : true
  })
};

@dominikg
Copy link
Member

dominikg commented Jul 12, 2024

but then each tool would have to check if compilerOptions is a function and call it first before passing the output to svelte.compile. It would be a bit more convenient and reduce duplication if there was a way to just pass compilerOptions to svelte.compile and have that figure it out. Last idea to that i have is adding a dynamic: ({filename,content,options})=>CompilerOptions to CompilerOptions itself, that svelte.compile invokes if present and merges the output back on options

edit: passing source/content to the dynamic function can help too if you want to inspect svelte:options to selectively enable custom element output for example.

@dummdidumm
Copy link
Member

dummdidumm commented Dec 3, 2024

Recapping the various solutions and their pros and cons:

Make each property a function

// svelte.config.js
export default {
  compilerOptions: {
    css: ({ filename }) => filename.includes('/og-cards/') ? 'injected' : 'external',
    runes: ({ filename }) => filename.includes('/node_modules/') ? undefined : true
  }
};

Pros:

  • Seems straightforward at first
  • Nice for when you only want a subset of options to be dynamic
  • Nice for when the option value's outcome need different logic for each
  • Can make this part of the compiler itself, meaning logic is encapsulated

Cons:

  • Cumbersome to write when the logic to go to value A or B is always the same (i.e. if you repeat filename.includes('x') everywhere)
  • Likely needs adjustments in tooling regardless since they do checks here and there along the lines of "if this option is set to X then force this other to Y, or warn"

Add dynamic option

// svelte.config.js
export default {
  compilerOptions: {
    css: 'injected',
    dynamic: ({ filename }) => ({ 
       runes: filename.includes('/node_modules/') ? undefined : true
    })
  }
};

Pros:

  • Seems straightforward at first
  • Nice for when you only want a subset of options to be dynamic
  • Can make this part of the compiler itself, meaning logic is encapsulated

Cons:

  • Likely needs adjustments in tooling regardless since they do checks here and there along the lines of "if this option is set to X then force this other to Y, or warn"
  • You can have a static option be overridden by a dynamic one, feels a bit weird

Make this a svelte.config.js thing with one function

// svelte.config.js
export default {
  compilerOptions: ({ filename }) => ({
    css: filename.includes('/og-cards/') ? 'injected' : 'external',
    runes: filename.includes('/node_modules/') ? undefined : true
  })
};

Pros:

  • Nice to write when the logic to go to value A or B is always the same (i.e. if you just do filename.includes('x') and return a bunch of stuff depending on that)
  • Compiler stays agnostic of filesystem shenanigans

Cons:

  • Every tooling needs to implement support for this

Proposal

Since every tooling needs to be likely adjusted anyway (for example, see this manipulation of the passed compiler options in v-p-s I propose to strictly go by "which of these APIs feels nicest", and therefore go with Option 3 and make it a svelte.config.js thing that every tooling needs to implement separately.

The question left to answer is what's passed to the dynamic compiler options.

  • filename is a no-brainer
  • code is a question-mark. It potentially makes things harder because if we take preprocessing into account then that becomes troublesome for I know that v-p-s' dynamicCompilerOptions does it. @bluwy / @dominikg do you know of any cases where it has proven useful / if there are any cases that can't be implemented without it?

@dominikg
Copy link
Member

dominikg commented Dec 4, 2024

my goto example for dynamicCompileOptions is to compile web components based on the presence of the tag option.

It also allows people to add their own frontmatter compileOptions or whatever. we have the code and the filename, the question is why shouldn't we pass the code?

sveltejs/vite-plugin-svelte#270 (comment)

@dummdidumm
Copy link
Member

I see, thanks.

If we do this, then the following tools need updates:

  • vite-plugin-svelte (@dominikg could you take this one once we decide on an API?)
  • rollup-plugin-svelte (I can take that one)
  • svelte-loader (I can take that one)
  • language-tools (I can take that one)
  • eslint? Not sure about this one

@dominikg
Copy link
Member

dominikg commented Dec 4, 2024

prettier plugin might need it too? tbh i'd still prefer if svelte itself took care or it instead of introducing the same/similar code in all of the above to make it work.

@dummdidumm
Copy link
Member

As pointed out in my post above, tooling needs to be adjusted anyway, because of how tooling sometimes manipulates the options, so this argument doesn't really apply IMO. For example, v-p-s modifies options here, though it seems like it does not do that after invoking dynamic compile options.

@dominikg
Copy link
Member

dominikg commented Dec 4, 2024

v-p-s is the exception here though as it needs to manage some options for better dev experience and builtin ssr/client support.

it would only have to do sth like

if(typeof options.compilerOptions === 'function'){
 const _compilerOptions = options.compilerOptions;
 options.compilerOptions = ({filename,code})=>{
   return {
     ..._compilerOptions({filename,code}),
     filename,
    cssHash: ...
    generate: ssr ? 'server': 'client'
   }
 }
}

and possibly deprecate its own dynamicCompileOptions option

@dominikg
Copy link
Member

dominikg commented Dec 4, 2024

if we make this a svelte.config.js thing we would have to think about reviving sveltejs/rfcs#59 or at least distribute a type and defineConfig util somewhere that helps users with authoring it.

@dummdidumm
Copy link
Member

Rich and Dominic also favor the compilerOptions: ({ filename, code }) => ({...}) option, so I'll give it a go on the other tooling.

How would a defineConfig util help us here (not just "nice to write" but also from a maintenance perspective, what could we save across packages)?

dummdidumm added a commit to sveltejs/language-tools that referenced this issue Dec 9, 2024
For svelte2tsx we gotta make a compromise - since everything has to be sync, we can only pass the unpreprocessed file. Should be fine in practise because a preprocessor influencing the code in such a way that it in turn influences the compiler options is probably vanishingly rare.

sveltejs/svelte#12415
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants