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

svelte.preprocess has no file path awareness #983

Closed
tivac opened this issue Dec 5, 2017 · 12 comments
Closed

svelte.preprocess has no file path awareness #983

tivac opened this issue Dec 5, 2017 · 12 comments

Comments

@tivac
Copy link
Contributor

tivac commented Dec 5, 2017

I am trying to get the new svelte.preprocess functionality working with my modular-css project, but currently none of the hooks know anything about the file they're operating on. This is problematic for anyone who wants to do things that could involve external files because it makes file resolution in any sane sort of way impossible.

I could solve this with a slightly gnarly closure variable for the case where I'm using svelte.preprocess directly, but I don't think that is very likely. I'm using rollup-plugin-svelte currently and have no desire to write my own kludgy thing instead if I can avoid it.

I have a plan that seems pretty achievable and I'd love to get your thoughts, @Rich-Harris.

I could modify svelte.preprocess so that each of the hooks got passed the options object along with the content to manipulate. That would make it possible to pass around more context, like the file path being manipulated. Then rollup-plugin-svelte would be modified to pass the id into the call to svelte.preprocess() and anyone who wants to manipulate the sections could do it with enough context to resolve @import statements or anything else they want.

Thoughts? I'm happy to fumble my way through a PR for this, I really want to get modular-css working with svelte.preprocess() so I can shrink our bundles!

@tivac
Copy link
Contributor Author

tivac commented Dec 5, 2017

Decided to investigate further, it seems pretty simple?

https://github.com/sveltejs/svelte/compare/master...tivac:preprocess-options?expand=1

@Rich-Harris
Copy link
Member

Yeah, should be straightforward. I wonder if we're better off being explicit about the kinds of options we're expecting here, and rather than passing the entire options object around, establish some clear conventions?

preprocess(source, {
  markup: ({ content, filename }) => {...},
  style: ({ content, attributes, filename }) => {...},
  script: ({ content, attributes, filename }) => {...},
  filename
});

It feels like that would be better in terms of having a clear and consistent approach across build tool integrations and preprocessors, though maybe by proposing a whitelist of 'special' properties I'm not being forward-looking?

(I'd envisage that preprocessor plugins would all accept options of their own, so you'd always have to invoke them — script: transformJs(transformerOptions) — which should lessen the need to pass around arbitrary options from preprocess to the individual preprocessors.)

@tivac
Copy link
Contributor Author

tivac commented Dec 5, 2017

I don't think being particular about properties is necessarily a problem, it's just limiting how other folks can (ab)use your APIs. That may be attractive or it may not depending on any number of factors.

I feel like I'm often trying to do something a bit "odd", so having generic ways to pass things around is usually valuable for me. If you want it to just be a specific property that can be extracted & then passed to the hooks I'm on board w/ that too.

@Rich-Harris
Copy link
Member

I reckon we should probably go with just filename for now, and revisit if it turns out we need to.

Something else we'll probably want to consider eventually — it'd be cool if preprocessors could return an array of dependencies along with code and map, so that rollup-plugin-svelte could pass it back up to Rollup, so that changes to variables.scss or whatever triggered a rebuild. (I mean, Rollup can't currently do that, but it would be nice if it could...)

@tivac
Copy link
Contributor Author

tivac commented Dec 6, 2017

Agreed, I worked around that limitation by having modular-css inject import statements for any files within the CSS dependency graph but having it built in would be nice.

@Rich-Harris
Copy link
Member

within the CASE dependency graph

CASE who?

@tivac
Copy link
Contributor Author

tivac commented Dec 6, 2017

Autocorrect on mobile, edited now but it was meant to say "CSS". 😞

@esarbanis
Copy link
Contributor

I may be off topic here, but isn't this what you are after?

@Rich-Harris
Copy link
Member

Ha. I thought I was about to learn some new graph terminology!

@esarbanis this is about being able to access filename inside the preprocessor functions when you're not invoking them directly (i.e. you've handed the functions off to e.g. rollup-plugin-svelte, and it's invoking them, meaning you don't know which file is being processed and therefore how to resolve relative import paths).

@tivac
Copy link
Contributor Author

tivac commented Dec 7, 2017

Got this working, eventually. It's a little wild. Example of usage:

https://gist.github.com/tivac/0c353a4e0fb32ab05e05a2eac26a1f24

Have to create a function that returns the preprocess args for rollup-plugin-svelte so I can get at the markup & rewrite it, but it also has to return a rollup plugin so I can use ongenerate/onwrite to output the CSS.

(This is where a standard API within rollup to add output files would be useful, assuming that rollup-plugin-svelte was able to make it available to the preprocess hooks somehow)

So that's gross, but it works! Took our bundle from 72k down to 67k, & the actual user-facing impact would be even better due to a lack of change detection code being parsed/run for our current completely-static CSS objects.

Note that this also depends on changes to rollup-plugin-svelte and svelte so that the filename is available to preprocess hooks.

@tivac
Copy link
Contributor Author

tivac commented Dec 7, 2017

@Rich-Harris would PRs for the changes to svelte & rollup-plugin-svelte be something you would merge?

@Rich-Harris
Copy link
Member

to add filename? yes, definitely

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

No branches or pull requests

3 participants