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

Module Unification Namespaces #309

Closed
wants to merge 1 commit into from
Closed

Conversation

mixonic
Copy link
Member

@mixonic mixonic commented Mar 2, 2018

@mixonic
Copy link
Member Author

mixonic commented Mar 2, 2018

This was discussed in the Friday core meeting today. Highlights of what we covered and some notes:

  • Implicit namespaces in templates
  • Implicit namespaces in services
    • Can we close over the addon scope and have a special service inject for the addon?
    • Can we make a mini-rfc for throwing a build-time error
  • Invoking from an NPM scope
    • wycats doesn’t like the @, wants to just permit w/o @
    • Can we rely on import down the line
    • Can we rely on the component helper down the line
    • has a cost, people won’t want to use scoped packages
  • ember- and ember-cli- stripping
    • We have new consensus to not do this
  • Namespace and source APIs on owner, and the limits of those APIs (no way to address factory specific injections, options)
  • Resolver API, implement via expandLocalLookup

I will make updates the the RFC today and leave a note when I do.

@mikkopaderes
Copy link

mikkopaderes commented Mar 3, 2018

Only services, components, and helpers from an addon can be resolved by an application.

Was this always the case? I'm currently developing an internal addon which will expose some models. There's no blueprint for it in Ember CLI but I can manually create them myself. It works but I'm worried if I should abandon that approach. If it should continue to work, will the invocation also change? e.g. store.findRecord('my-name-space::user')

My use-case is that I have multiple apps that uses a single database. The models are mostly equivalent for each app so I believe that moving the models in an addon would be our best bet to keep things DRY.

@mixonic
Copy link
Member Author

mixonic commented Mar 3, 2018

@rmmmp Regarding your specific use case: I do not suggest any change to Ember Data here. The RFC provides very specific examples of where the :: syntax can be used. There is no implication you could pass it to findRecord, for example.

This RFC does outline support at the low level (factoryFor for example) for namespaces. Ember Data could choose to support namespaced models using the :: syntax, and this doesn't explicitly disallow that.

However it also doesn't rush to add that support. First, it would be basically impossible to make all of Ember and it's various libraries work with :: syntax. I'd like to keep our progress iterative. Second, I consider the :: syntax a medium-term solution. Other parts of the Ember team have discussed formalizing absolute specifiers (a canonical ID for a given resolution) or leaning more on the naive ES import system to reference factories. That motivates the limited design space of this RFC.

@mixonic
Copy link
Member Author

mixonic commented Mar 3, 2018

The RFC has been updated:

  • Invoking something from an NPM scoped package now requires use of the {{component helper. It is not suggested that you can invoke these packages directly as {{@scope/package::name}} since the leading @ leads to some confusing wrt @arg, and because the / makes a closing tag difficult to process visually.
  • Stripping of ember- and ember-cli is explicitly removed, and change compared to the original module unification RFC. Experienced developers would appreciate the stripping behavior, but new developers would certainly find it confusing and surprising.

@Gaurav0
Copy link
Contributor

Gaurav0 commented Mar 4, 2018

Only services, components, and helpers from an addon can be resolved by an application.

I think the reasoning behind this should be explicitly explained and added to the RFC. I think there are many use cases for this:

Some addons add new type(s) that must be automatically resolved. A prominent example is ember-data. With this limitation, they could not namespace implementations of those types.

A major enterprise company I used to work for chose to extend the DSL for routing. In order to implement, they created a custom resolver with virtual types which resolved to "cards" rather than routes. With this limitation, they could not combine this approach with namespaces.

Glimmer Components and other template invoked types that have not yet been fully specified may also require this.

@ro0gr
Copy link

ro0gr commented Mar 9, 2018

I'm a bit worried about an idea to write in a such verbose way on a regular base:

{{#package-name::component-name as |smth|}}
  ...
{{/package-name::component-name}}

I think the most annoying part is a closing tag: {{/package-name::component-name}} .

One of the regular build error I get is related to an invalid closing tag. With a package name it becomes even more complicated/error-prone to write. Maybe good tooling may help here.

Which may be a hypothetical angle bracket syntax?

<some-package::MyCoolComponent>
</some-package::MyCoolComponent>

I'm cool with a necessity to be specific in js but I'd prefer to leave templates ergonomic. If I have an ability to import namespaces I would choose this way.


Fallback to main may reduce a verbosity. But I think there always will be "UI components" packages like "ember-bootstrap" which have a rich set of components.

@sandstrom
Copy link
Contributor

@mixonic Will there be an easy way of importing a package, say wildlife and then choose to make it available to the app as wild instead?

In other words, are we forced to use the NPM package name, or can we 'rename' them on import (something akin to JS modules import foo as bar from 'foo').

@csantero
Copy link

We have a large library of components in several npm-scoped addons, and it's very frequent in our apps' templates to see, given a component called standard-table provided by an addon called @my-namespace/my-addon:

   {{#standard-table}}
   {{/standard-table}}

Under module unification, it appears that we'd have to invoke this as:

    {{#component "@my-namespace/my-addon::standard-table"}}
    {{/component}}

I think it's safe to say nobody on our team is going to be happy to have to do this.

@sandstrom's suggestion would be very helpful for us. For example, if we could globally map @my-namespace/my-addon to just my-addon, then we could do:

    {{#my-addon::standard-table}}
    {{/my-addon::standard-table}}

@mikkopaderes
Copy link

I second @csantero's concern. We're going to have the same issues in our apps and I can definitely say that it's not gonna be a fun experience for us.

@mixonic
Copy link
Member Author

mixonic commented Mar 16, 2018

I want to thank people for their feedback here and I encourage others to continue adding their reviews. EmberConf has kept me a bit busy 😬 but I have updates I want to make this weekend and I'll reply to comments as well. Thanks for your patience.

@tim-evans
Copy link

tim-evans commented Mar 16, 2018

Would it make sense to remove the npm scope if the app is in the same npm scope?

For example, at Condé Nast, we have a bunch of npm packages published with the @condenast scope. If our application @condenast/cms has the same scope as our addon library @condenast/ember-ui, would it be sensible to drop @condenast?

My concern is less that the {{component}} invocation syntax version is too long, and more that it decreases the readability of the code. The approach using a helper, at first glance, seems to decrease the readability and clarity.

My suggestion above may not be appropriate, and have clarity issues for developers.

Am I thinking that there may be an {{import}} helper to resolve these scope issues?

cf

{{#import '@condenast/ember-ui' as |ui|}}
  {{#ui::button}}button{{/ui::button}}
{{/import}}

@csantero
Copy link

Would it make sense to remove the npm scope if the app is in the same npm scope?

@tim-evans That wouldn't actually work for our use-case since our apps (unlike our addons) don't use an npm scope. I guess we could change it but that would involve modifying a lot of files, as every ES6 import of app-defined modules would have to be rewritten.

@gossi
Copy link

gossi commented Jul 6, 2018

With all this explicitely I have strong tendency we are heading into a world where we are forced to read and write idiotic templates. MU and angle brackets for me are about clarity (file/folder-wise) and catching up to web components of just writing <MyComponent> and we are good to go. Addons are a good way for shared code, especially components. Now, if using a component forces me to write:

{{use my-name as MyName from '@scoped/pkg-name::my-name'}}
<MyName/>

then this is not a world, where I just can go with <MyName>. It may be more explicit in case another developer might be unclear about where that component is coming from, sure. Though for me as the main developer it will become very frustratring and exhausting repeating these imports over and over again. The DX will suffer and decrease with all these non-enjoyable-complex-explicit-statements.

The question for myself will rise, if I develop a MU or classic addon? MU for developing clarity of the new file structure or classic addon for an enjoyable consume of my addon/components of it?

In order to keep up DX and joy, while also denying auto-imported components and keep a bit of explicity by manually declaring imported components (helpers, services, ..., ?) with the chance of aliasing them (in case I depend on two addons that both have a component of the same name).
The idea is, to allow for a configuration file at application level where I can map imports to their desired names.

In a slack discussion with @knownasilya @lifeart and @ef4 this could be at config/aliases.js rsp. config/aliases/(components,helpers,services).js and could look like this:

// config/aliases.js

module.exports = {
  '@myorg/myaddon': 'sausage',
  '@myorg/myaddon/components': {
    'my-component': 'MyComponent',
    'my-other-component': 'SomethingCompletelyDifferent'
  }
}

So you can still do <sausage::table>.

Also @ef4 suggested, this could be an addon at first, though required APIs should exist then.

@ef4
Copy link
Contributor

ef4 commented Jul 6, 2018

Another option for what @gossi is describing (whether as an addon or an eventually first-class feature) would be to create a preamble template. A preamble template would be an hbs file that works as if it was pasted into the beginning of all your other templates.

This has the benefit of not introducing a second syntax for doing the same thing. Your preamble would be able to just say {{use my-name as MyName from '@scoped/pkg-name::my-name'}} to make MyName available throughout the app.

This can obviously be abused, and I would be more comfortable shipping it as an addon first and seeing how people use it in real projects before making it a standard feature.

@ro0gr
Copy link

ro0gr commented Jul 18, 2018

A no sense suggestion

I think today we have concept which looks similar to use. Contextual components is a way to yield configured components:

{{#my-addon as |i|}}
  {{i.header}}
  {{i.body}}
  {{i.footer}}
{{/my-addon}}

It may be a bad idea but what if in MU world default components are supplied with a namespaced components?

The only thing I don't like about the current way to access context is a reliance on order, so there is an imagined destruction syntax with angle bracket:

<SomeAddon as { A } >
  <A>
</SomeAddon>

or even an inline(simple) form:

<SomeAddon as { A } />
<AnotherAddon as { B } />

<A />
<B />

I feel there are lots of edges but I like this syntax.

Just realized my examples are wrong in a different ways(sorry for the noise). Collapsed it to not pollute the thread.

@mixonic
Copy link
Member Author

mixonic commented Aug 27, 2018

Closed in favor of #367

@mixonic mixonic closed this Aug 27, 2018
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.

9 participants