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

WIP: "interface" for exported properties #3917

Closed
wants to merge 2 commits into from

Conversation

stalkerg
Copy link
Contributor

Firstly, this born as a reaction to that PR #3802.
Situation:
I am using ASR routing, this solution put "asr" property into any component and very often I have no reason to use it inside a component. Also, ASR aggregate params from all routing levels and push down it and set as props, and these props also often not needed in my component.
If I remove export let asr; I will see a warning in the browser because you will try to set not existent props. But if I keep this export, after #3802 I will see warnings during compile.

Idea:
If I would have access to a list of export properties I can decide outside component what exactly I can set. Author of ASR @TehShrike also thought about such a feature.
But this "interface" provides much more opportunities. Now, you can request and prepare all data what exactly needs for such a component because you will know the public interface of this component.
Probably it's not needed if you use a component inside another component.
But looks very helpful for the case when you directly call the constructor.

@Rich-Harris
Copy link
Member

As a general rule we want to be very careful about any API changes or anything that results in increased code being generated — it has to cross a very high bar. I'm not sure if avoiding ASR warnings meets that bar.

Can you elaborate on how and why this warning is appearing, e.g. with a repro?

There are a couple of workarounds:

  • export let asr = undefined will squelch the warning
  • Using a custom onwarn compiler option would allow you to filter the warning out

@stalkerg
Copy link
Contributor Author

As a general rule we want to be very careful about any API changes or anything that results in increased code being generated — it has to cross a very high bar.

I 100% agree with you, this is a very sensitive change. It's why I set it as WIP.

I'm not sure if avoiding ASR warnings meets that bar.

It's not only for ASR. Basically, it's also about any case when you can't control props to create a component.

Can you elaborate on how and why this warning is appearing

Sure, it's simple. In the debug mode Sevelte generates code for validate input props. If such a key did not exist as an export property in the component we saw such a warning.
You introduce this feature here #2840.

e.g. with a repro?

If you still want it I can prepare it.

export let asr = undefined will squelch the warning

WOW, it's can solve most of the problems but still can't help if I not expected such property.
Also, this workaround pushes me to bloat my component interface.

Using a custom onwarn compiler option would allow you to filter the warning out

no, for most cases I still want to see such a warning, it has already helped me to remove dead code.

Thank you for your time. I hope my explanation enough for such a situation.

@stalkerg
Copy link
Contributor Author

PS I suppose will be good to separate somehow components that we use internally in Svelte and component what we directly create outside Svelte stack. Such components should have a rich public API for simple integration. "increased code" it will be not so huge problem for such components. It's just an idea, I wrote it to keep for children... :)

@rixo
Copy link
Contributor

rixo commented Nov 15, 2019

In my opinion, for the use case & expected benefits you're citing, your problems arise from not using the right tools for the job.

You explain very well that only the component can know which data it will need, and you suggest a way for the component to publish this information to the outside world, so that the provider can decide what props to inject accordingly.

In fact, you want the consumer (component) to "pull" in only the data it needs ("lazily"). But you are using props, that are inherently a "push" ("greedy") construct, like function arguments. When you push, it's the provider's responsibility to know what to provide. And when it provides something that is not consumed, that's spoil. In this respect, Svelte's warnings are spot on. They do signal a real flaw in the design.

Fortunately, Svelte already has all what is needed to implement an elegant pull solution to this pull problem, with import, context & stores. And then you won't have a warning problem, because there will be nothing amiss to warn about.

// you pull only what you need
import { getRouterContext } from 'pull-router'

// what you pull can depend on where your component instance is sitting
// in the tree (i.e. be contextual, cf. getContext / setContext)
const { url, params } = getRouterContext()

// what you pull can be "static" (i.e. its value does not change during 
// component's lifecycle)
const homeUrl = url('/')

// or it can be reactive
$: message = "Hello " + $params.name

In the provider, you'd use setContext / getContext and svelte/store to cook a good little API for the consumer, and everybody be golden.

For completeness, here's the code for avoiding compiler warnings with this approach, in a component that does not use these data (your initial problem):

// I don't get no warning for not importing nothing

With existing tools, we can already completely avoid the problem and the need for a new solution and its associated cost. Not to mention that the existing tools are so stunningly easy to write & use that it will actually be very hard to come up with a better proposal. Just pick the right tools.

cc @TehShrike @jakobrosenberg Hey, router guys! Any opinion on this? Would you be interested in providing "pull only" solutions to access contextual data from your routers? Is there a clear advantage to the prop injection scheme that I missed?

@stalkerg Aditionnally, you can already provide all the meta data you want for a "rich API" component by exporting things from <script context="module">.

@TehShrike
Copy link
Member

Is component context an appropriate solution for dependency injection?

From what I can tell, ASR would implement that by doing something like

const component = new Component(options)
component.$$.context.set('asr', { makePath, stateIsActive })

but that wouldn't solve the issue, because these functions need to be available during the first render.

If there was some way to pass contexts to component constructors, that would be a cool way to do dependency injection. It would be great to be able to take advantage of context inheritance.

@rixo
Copy link
Contributor

rixo commented Nov 16, 2019

Just a quick follow up to be sure nobody loses time answering my previous post... Scratch that, I was wrong. I dig some digging with ASR and I don't think my previous position holds. Coming back with more details when time allows.

@halfnelson
Copy link
Contributor

halfnelson commented Nov 16, 2019

I think this is a job for context.
Looking at the implementation of the ASR support for svelte,
at this line
https://github.com/TehShrike/svelte-state-renderer/blob/master/index.js#L19
it just constructs the selected component.

If instead we instantiate a wrapper component that sets the context, then the components can "opt in"

Example here:
https://svelte.dev/repl/706d6a5aedfd4df2a91b97202be74a03?version=3.14.1

Free bonus with this, is that ancestors get access to the route props too

@rixo
Copy link
Contributor

rixo commented Nov 16, 2019

Yup. I tried something along those lines yesterday.

It works. It's great in some respects, like having all components in the same context.

But still... In the end, from a user perspective, I think props are the most pleasant API to have in some cases. In particular for data that are resolved / injected by the router. For those, the warnings are actually desirable if the expectations of the component are not respected (missing expected prop) but not for extraneous props (that may come to parents in the case of ASR). You lose that when you switch to context.

So I think some kind of warning filtering would be desirable. In fact I kind of need it myself: #3822

Since the "too much warning" problem is dev-only, the solution should probably be dev only too. This way it won't impact bundle size.

I keep wondering if it would make sense to expose the whole vars object returned by the compiler during dev. It would enable support of these kind of things in userland. It could help for things like HMR too (to perfectly emulate the component API when it changes). Currently, waiting for dedicated hooks, the HMR plugin does this (it attaches the vars object to component classes).

@jakobrosenberg
Copy link

I'm a little late to the party, but here's what I did to deal with the warnings when working on Svelte-filerouter.

module.exports.suppressWarnings = (function () {
    const _ignoreList = []
    let initialized = false
    const _warn = console.warn

    return function (newIgnores = []) {
        newIgnores.forEach(key => {
            if (!_ignoreList.includes(key))
                _ignoreList.push(key)
        })

        if (!initialized) {
            initialized = true
            console.warn = function (...params) {
                const msg = params[0]
                const match = _ignoreList.filter(prop => msg.match(new RegExp(`was created with unknown prop '${prop}'`))).length
                if (!match)
                    _warn(...params)
            }
        }
    }
})()

Basically the router checks which props are passed to the next child and adds these props to the list of warnings to be suppressed.

@halfnelson
Copy link
Contributor

halfnelson commented Nov 16, 2019

@rixo wrote:

You lose that when you switch to context.

In the example I posted it was the intention that the resolved route variables for the property would be injected as props, and the asr helpers that would be fetched from context. Which means if you had /blog-post/:slug your Blog component would get <Blog slug={slug}> then inside Blog if you want the asr goodies you could getRouteContext as opt in.

@rixo
Copy link
Contributor

rixo commented Nov 17, 2019

@TehShrike I've begun implementation of a context enabled renderer. (I won't have the time to finish this anytime soon, unfortunately :-/)

@jakobrosenberg That's a little bit scary TBH, but that's probably the way to go until there's a better way.

@halfnelson Yes, that's also how I view it. But even so you still have some "opt-in" props. Like for example parameters from parent routes that the child might want to access, or not. In the case of ASR, this effect is amplified because it not only exposes route params but also arbitrary data loaded for the route (and from parent routes...).

All in all, I (now) think the problem targeted in the PR is valid and should be solved.

The more I think about it, the more I believe that the right solution would be to expose the vars object from the compiler in components, in dev mode.

It would solve the problem here, the same problem in svelte-filerouter, and the unrelated problem of emulating user components' API in HMR. My intuition is that it would enable many other use cases over time. The drawback, as compared to the solution in this PR, is that this would be dev mode only. But AFAICT we currently have no identified use case outside of dev mode. And that saves us from having to worry to much about performance.

Maybe as an opt-in feature of dev mode, since it might add a non-negligible overhead. But maybe not... Like I said, I already do that currently when using HMR and never noticed a problem related to it. Maybe opt-out.

@stalkerg What do you think? Would this solution work for your problem?

@jakobrosenberg
Copy link

jakobrosenberg commented Nov 17, 2019

@jakobrosenberg That's a little bit scary TBH, but that's probably the way to go until there's a better way.

Much agree, but beggars can't be choosers. I think the only other option would be dynamically using setContext in the "walker" component, which has its own set of downsides.

I know very little of the internals of Svelte, but I get the impression that much greater control could be given to developers. Something I'm all for. It may not matter much to most devs, but for those working on the Svelte ecosystem it means the world.

@rixo
Copy link
Contributor

rixo commented Nov 18, 2019

I've gone wild and implemented the rest of my ASR renderer & example (unrelated: with support for context & slots!): https://github.com/rixo/svelte-template-hot/tree/abstract-state-router

For POC purpose, I've also implemented the solution I proposed above, of exposing compile result vars in dev mode. For that, I've added support for the proposal (as Component.$compile for now) through rollup-plugin-svelte-hot, and used that in my ASR renderer to filter the props that are expected by the component. It seems to work.

I don't know if that would make sense to implement this experimental solution, supported only by my hot bundler forks, in other routers / ASR renderers.

@stalkerg
Copy link
Contributor Author

@rixo thanks for that detail discussion here!

What do you think? Would this solution work for your problem?

Yes, I suppose it solves all my problems but at the same time maybe better add a special component option for that because sometimes it's can be helpful even for production builds (by my intuition).

@rixo
Copy link
Contributor

rixo commented Nov 19, 2019

maybe better add a special component option for that because sometimes it's can be helpful even for production builds (by my intuition)

@stalkerg You make me realize that, in fact, there's already an option that's pretty close to that. If you set accessors: true on your component, then it becomes possible to introspect the component's props without having to instantiate it, even in non-dev mode. REPL example

With this, it should be possible to implement warning suppression in routers today -- even though it's probably not the proper long-term solution in this case, since it's a dev mode only problem, and it should be solved only in dev mode (and not require a special option to be solved).

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.

7 participants