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

Reduce boilerplate when importing components #1038

Closed
PaulBGD opened this issue Dec 22, 2017 · 30 comments
Closed

Reduce boilerplate when importing components #1038

PaulBGD opened this issue Dec 22, 2017 · 30 comments

Comments

@PaulBGD
Copy link
Member

PaulBGD commented Dec 22, 2017

This is just a thought while writing a lot of components, usually I'm importing a component just to pass it to the components object that I'm exporting. What if we trimmed down the boilerplate and had the compiler add the import statement for us? An example would look like:

export default {
  components: {
    "MyButton": "./forms/button"
  }
};

Which would get converted to

import MyButton from './forms/button';

export default {
  components: {
    MyButton
  }
};

We can still have backwards compatibility by only checking if a string key/value pairis passed and if we import files in the wrong order, the user can still manually specify the import order.

@Rich-Harris
Copy link
Member

Oh, I like that. Can't really think of any downsides

@roman-vanesyan
Copy link

Personally I'm against it. IMHO using explicit import seems more natural which leading to faster and better understand of the code. Plus, what re-inverting the wheels for, why add another bytes of code to svelte, if there're already tools that can deal with import well?

@PaulBGD
Copy link
Member Author

PaulBGD commented Dec 25, 2017

I'm a huge believer in having no magic in a codebase. However I do think that since the export object is already known to be transpiled by Svelte, it makes sense that it could handle things like automatically importing components.

As for other tools, this doesn't handle the import. It actually adds the import so other tools know where to import the component.

@lnryan
Copy link

lnryan commented Dec 25, 2017

I've long (in ractive) wanted the same thing, but would go a step further. I see no reason to have to have components: { 'MyButton' } when <MyButton> can be identified as one. And if this were tricky to implement, could we consider <comp:MyButton>?

Personally, I think there's a difference between 'magic' and reducing coupling: having to explicitly specify something that follows the internal logic of the overall solution (Svelte) doesn't add value and introduces a potential point of inconsistency.

If I use a component: , I also have to remember to list it as something I use or it doesn't work. If I replace or rename MyButton with OurButton, I now also have to remember to replace it in the declaration (and currently the import).

My feeling is any time you have to add explicit linking declarations they should serve a purpose of extending the core functionality, not simply making the core functionality work.

So, this serves a purpose of saying, hey, this component is sourced outside my core app

   components: { 'MyButton':'../forms/Button.html' }

but

   components: { MyButton }

doesn't tell us anything we didn't already know.

@lnryan
Copy link

lnryan commented Dec 25, 2017

Also, is there a way to use namespaces with Svelte. I've seen this in other frameworks for libraries and this would be very helpful with building a suite of applications with common UI / behaviors. For example if I have 4 small applications for various data-entry tasks, I'd really like to be able to use the same library of Svelte forms, login components, etc. In this scenario, it would be great to be able to do something like this:

namespaces: { 'core':Core } where Core is a node module i could import and add to my node dependencies...

or
namespaces: { 'core':'[path/to/cdn' } where it's hosted elsewhere

@emilos
Copy link
Contributor

emilos commented Dec 26, 2017

I think that the syntax:

components: {
   Button: './components/Button'
}

might cause some confusion if someone decides to do things like:

components: ['Button', 'Checkbox'].reduce((previous, current) => {
  previous[current] = `./components/${current}.html`
  return previous
}, {})

and it leaves us with a question if the above should be supported too or not.

Personally I wouldn't mind an alternative such as:

<link rel='import' name='Button' href='./components/Button.html'>

so that I could keep track of my components in the template only.

@paulovieira
Copy link

This would be nice improvement to clean the code a bit. I've always found redundant the need to have the "components" property.

Either

// 1 - automatic look-up from the imported moduled

<MyButton />

<script>

import MyButton from './forms/button';

export default {
    data: {}
}

</script>

or

// 2 - add the import behind the scenes

<MyButton />

<script>

export default {
    data: {},
    components: {
        MyButton: './forms/button'
    }
}

</script>

will make the code cleaner.

I think the first one would be better because it would still allow to use "components" as we know it today, which could be used to override the automatic look-up from the imports.

That is, if "components" is used, it must have a reference to all the components used in the template. This would still allow (?) the usage of more specialized cases like some sort some dynamic processing...

@PaulBGD
Copy link
Member Author

PaulBGD commented Dec 27, 2017

@paulovieira

// 1 - automatic look-up from the imported moduled

<MyButton />

<script>

import MyButton from './forms/button';

export default {
    data: {}
}

</script>

The issue with this is that MyButton is just a random value, Svelte doesn't know if it's a component or not and cannot know in most cases if it should use it as a component.

@emilos

components: ['Button', 'Checkbox'].reduce((previous, current) => {
  previous[current] = `./components/${current}.html`
  return previous
}, {})

This isn't statically analyzable, so this shouldn't be allowed in the first place.

<link rel='import' name='Button' href='./components/Button.html'>

Now this is interesting. It's a single line, easy to copy/paste, and very easy to read/analyze. I do think leaving importing/defining other components to the JavaScript is where it belongs, but I'm not too against this syntax.

@lnryan

Personally, I think there's a difference between 'magic' and reducing coupling: having to explicitly specify something that follows the internal logic of the overall solution (Svelte) doesn't add value and introduces a potential point of inconsistency.

If I use a component: , I also have to remember to list it as something I use or it doesn't work. If I replace or rename MyButton with OurButton, I now also have to remember to replace it in the declaration (and currently the import).

I like your argument, but reducing coupling and adding magic aren't mutually exclusive. While it may be more convenient to just automatically import references components, that's a lot of lookup magic that's hard to reason with as a developer.

Also, is there a way to use namespaces with Svelte. I've seen this in other frameworks for libraries and this would be very helpful with building a suite of applications with common UI / behaviors. For example if I have 4 small applications for various data-entry tasks, I'd really like to be able to use the same library of Svelte forms, login components, etc. In this scenario, it would be great to be able to do something like this:

Now this is an interesting idea. If I used

<forms:input:InputField />

in my code with the directory format of

MyComponent.svelte
forms
  |  input
     |  InputField.svelte

There wouldn't be any magic as it's directly referencing the path to import with. It's very Java-y, but I do like the idea.

@Kiho
Copy link
Contributor

Kiho commented Dec 31, 2017

I don't need "MyButton": "./forms/button"
but I really like to have MyButton: await import('./forms/button')

@Draggha
Copy link

Draggha commented Jan 2, 2018

@Kiho that looks like async components to me. It would be awesome to have something like that in Svelte! This could even further be expanded by streamed server side rendering. *.*

@PaulBGD

Now this is interesting. It's a single line, easy to copy/paste, and very easy to read/analyze. I do think leaving importing/defining other components to the JavaScript is where it belongs, but I'm not too against this syntax.

What about a "special global framework tag" á là <:Self> or <:Window>?
E.g.:

<:Import type="component" from="components/Button" />

<Button />

The components name could be derived by the filename. Another property could be used to rename the component locally:

<:Import type="component" from="components/Button" as="MyButton" />

<MyButton />

Of course you could also add helpers to the list of importable "template extensions":

<:Import type="helper" from="_helpers/leftPad" as="format" />

<p>
  The time is
  <strong>{{hours}}:{{format(minutes, 2, '0')}}:{{format(seconds, 2, '0')}}</strong>
</p>

The JavaScript way of importing components and especially helpers would of course still be preferred when you need partial application, reflection or whatever else you might need. In any other case the above syntax extension could save a few key strokes.

Even async components and splittable imports like with the import() function could be achievable through another tag like <:AsyncImport>.
I could then express @Kiho 's propsal like this:

<:AsyncImport type="component" from="./forms/button" as ="MyButton" />

<MyButton />

@dschulten
Copy link

@lnryan

Also, is there a way to use namespaces with Svelte.

not sure about your exact use case and if you use a bundler, but rollup creates a namespace for your component package if you define output.name; also, you need to export the svelte components in the bundle's entry point defined by the ˋinputˋoption to make them available in the namespace.

@Draggha
Copy link

Draggha commented Jan 3, 2018

Sorry @.all, after a good night's sleep I realized how much work my proposal above would have entailed and that it would open a can of worms:
With an import statement inside of your templating language you'd have to guard it against usage inside of if, each and the like. Or you'd have to change how they work inside of them (like compiling into the import() function). That would be too much work for a "shorthand syntax". And even then the initial proposal from @PaulBGD would still be more concise.

@PaulBGD
Copy link
Member Author

PaulBGD commented Jan 4, 2018

I do like the idea of using standards and doing something like

MyButton: await import('./forms/button')

but this would be a big change to the current component system.

@Draggha
Copy link

Draggha commented Jan 6, 2018

@PaulBGD What about something like this?

{{#await getButton}}
  <Loader />
{{then MyButton}}
  <MyButton>Click me</MyButton>
{{catch error}}
  <p>Sorry, we are short on buttons today.</p>
{{/await}}

<script>
  export default {
    components: {
      "Loader": "./base/loader",
      "Toast": "./base/toast"
    },
    asyncComponents: {
      "getButton": import('./forms/button')
    }
  };
</script>

Seems a bit off, but I tried to reuse as much of the current syntax as possible.

@Conduitry
Copy link
Member

I think this issue has gotten a bit off the original path. The initial proposal was for something fairly straightforward to implement, and was essentially syntactic sugar - unlike many of the later things suggested here. Paul's suggested syntax bothered be a bit at first (I can't quite put my finger on why), but it seems quite practical. I may take a stab at implementing that later today.

@arxpoetica
Copy link
Member

arxpoetica commented Jan 7, 2018

Throwing in my opinion. DON'T DO THIS in native Svelte. It's too magical. I'm a fan of declarative, and think it should stay. This could easily be offported to a rollup plugin if people want it that badly.

Update: see below

@paulovieira
Copy link

Well, svelte could still support both ways of declaring the components being used: the explicit one (using "import" explicitely) and the shorter one.

The spirit of this feature is similar to how nodejs finds modules (the resolution algorithm, or the fact that there's no need to explicitely write ".js"). That's also a bit magical, and yet everyone uses it.

@arxpoetica
Copy link
Member

arxpoetica commented Jan 7, 2018

@paulovieira, yes, but that's also native to NPM. I don't like this proposal because it breaks with native JS import/export use, so anytime a newcomer comes along it might be the first thing they ask. If we stick to native, all we have to do is point them to ES docs.

Update: see below

@arxpoetica
Copy link
Member

Sorry, I meant *native to Node. (That is, expected behavior.)

@arxpoetica
Copy link
Member

arxpoetica commented Jan 20, 2018

Note quickly there are a few comments on this as well in (closed/duplicate) #1120. I might actually like the proposal from @pushqrdx of leaving the import in place, but convention being that as long as the HTML has the component and the JS has an import, it uses a little bit of magic instead of also saying, hey, component, use this.

Update: see below

@Conduitry
Copy link
Member

Yeah I think I also like the idea of leaving the import there and dropping the need for component, which is something that's come up before. But it does have a couple of problems:

  • we don't actually enforce yet that tags begin with a capital if and only if they are nested components (it's merely a suggestion)
  • linters will complain about unused variables
  • probably other stuff? haven't looked too closely into what warnings Svelte gives about unused components

While typing this, I just noticed that this suggestion also came up earlier in this thread. I still don't know whether I prefer @PaulBGD's original suggestion or the one of automatically hooking up components.

@pushqrdx
Copy link

@arxpoetica Well there is a big difference between "magic" and "redundancy" the whole point here is that you shouldn't have to specify the component's name 3 times in order to use it. That's plain nonsense. Also if you already have a component tag added to the html markup and you imported it down there is there any other reason the compiler shouldn't understand that this component is already used there, that's one point of having a compiler..

@teintinu
Copy link

teintinu commented Feb 9, 2018

My suggestion is just

import  './forms/button.html';  // the name identifier will be button, the same of file name
// or
import MyButton from  './forms/button.html';  // the name identifier will be MyButton

I'd try to make a PR #1167 for my implementations but the tests failed on CI. I'll work more if you like that propoval.

@arxpoetica
Copy link
Member

arxpoetica commented Feb 10, 2018

I admit I'm still on the fence about this. For example, @thr0w's solution creates a problem for many developers with ES linting parsers built in their coding environments. Having an unused MyButton variable will throw a linting error, leaving that ghastly red wiggly line. For some people, that means they will actually have to throw extra inline linting ignores for each failing line.

I'm officially of the opinion we should throw this whole ticket out, baby and bath water (sorry baby). I think it actually makes sense to keep the variable literals effect since it follows native JavaScript conventions.

Update: after talking about it in Gitter, I think I'm more in favor of the original proposal in this ticket. @Rich-Harris talked me down from off the ledge. 😄

@pushqrdx
Copy link

hope it gets implemented, yeah the original suggestion seems the most logical and clean

@Conduitry
Copy link
Member

Conduitry commented Apr 18, 2018

I was just taking a look at this because I'm interested in having it, but also because I want to get more familiar with the more recent codebase changes.

A super clunky implementation was actually quite easy. Add

if (node.type === 'Literal' && typeof node.value === 'string' && disambiguator === 'components' && !this.options.format || this.options.format === 'es') {
	componentDefinition.addBlock(`import ${name} from ${JSON.stringify(node.value)}`);
	return;
}

here. This implementation does suggest some other more specific questions:

  • Do we want implicit imports for other types of values besides components? Components are likely to be the most-used, and also obviously we can't use this for things where the actual value could be a string.
  • Do we want to support this in output modes other than ES? My gut instinct is that we want to support CJS but that's it. If someone tried to compile a component using this feature to another type, they should get an error - but I'm not sure whether we want to throw that from a validator or from right here in addDeclaration.
  • It would be nice to be able to use the existing generator.imports array for this and simply add items to it, but I don't know how practical that would be, because for ES code generation, it's relying on these being real estree nodes and it directly copies the corresponding slice of the input source, which won't exist.

edit: Okay it's not actually quite as simple as I made it out above, but I still do not see this being particularly challenging.

@Conduitry
Copy link
Member

Just pushed a moderately less clunky version to gh-1038 (based on v2) but it could use some more work (not to mention some tests). It's only supporting this shorthand for components, currently. I think what I wrote should work for all output modes but I've only tried it with ES and CJS. It introduces an additional array generator.implicitImports which is then used by wrapModule.

@hperrin
Copy link
Contributor

hperrin commented Apr 20, 2018

@Conduitry I use a lot of static properties, so I need the ability to still import Blah from './Blah.html';. Does your PR still allow that?

@Conduitry
Copy link
Member

Yep you can still use the regular syntax. This just makes it so that if you specify a literal string as a value for key in the components object, it adds an import for that path in the output.

Rich-Harris added a commit that referenced this issue Apr 20, 2018
Add support for shorthand imports of components
@Conduitry
Copy link
Member

Implemented in 2.1.0.

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