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

Document mappings.json format. #53

Merged
merged 8 commits into from
Apr 24, 2018

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Apr 23, 2018

  • This properly documents the existing mappings.json format
  • Ensures that all properties (other than replacement) are present on all entries
  • Updates the README with the interface:
  • Creates a new script that will allow easier formatting of the mappings.json (not terribly important, but it was nice to have as I made these changes)
  • Added tests to ensure that the interface is satisfied

interface Mapping {
    /**
      The globals based API that this module and export replace.
     */
    global: string;

    /**
      The module to import.
     */
    module: string;

    /**
      The export name from the module.
     */
    export: string;

    /**
      `true` if this module / export combination has been deprecated.
     */
    deprecated: boolean;

    /**
      The recommended `localName` to use for a given module/export.

      This is useful for things like ember-modules-codemod or eslint-plugin-ember
      so that they can provide a nice suggested import for a given global path usage.
     */
    localName: string;

    replacement?: {
        module: string;
        export: string;
    }
}

@rwjblue rwjblue changed the title Reformat mappings.json. Document mappings.json format. Apr 23, 2018
@rwjblue rwjblue requested a review from Turbo87 April 23, 2018 22:04
@Turbo87
Copy link
Member

Turbo87 commented Apr 24, 2018

what's the reason for making localName required?

@rwjblue
Copy link
Member Author

rwjblue commented Apr 24, 2018

  1. it’s really just “suggested local name”, obviously anyone can use whatever they want (and when I do an update for 0.4.0 with the getter / setter stuff I was going to change it to recommendedLocalName to make that even clearer)
  2. calculating and curating the default here prevents every downstream tool from having to deal with the reserved names manually (and possibly incorrectly). For example, eslint-plugin-ember does not seem to take reserved words into account at all when providing its suggestion.

tldr; it’s “free” to add and centralizing makes life a tad bit easier for downstreams.

@Turbo87
Copy link
Member

Turbo87 commented Apr 24, 2018

it’s really just “suggested local name”, obviously anyone can use whatever they want (and when I do an update for 0.4.0 with the getter / setter stuff I was going to change it to recommendedLocalName to make that even clearer)

renaming seems fine to me

calculating and curating the default here prevents every downstream tool from having to deal with the reserved names manually (and possibly incorrectly). For example, eslint-plugin-ember does not seem to take reserved words into account at all when providing its suggestion.

If we use e.g. localName: Object we should change to localName: EmberObject so that downstream projects don't have that problem, but I don't see how that relates to making localName a required key 🤔

I'm not hard blocking on this, just interested what the reasons are.

@rwjblue
Copy link
Member Author

rwjblue commented Apr 24, 2018

I don't feel super strongly. I mostly auto-populated it because it seemed easier to define a consistent interface (vs making localName optional) and it removes the extra "local building" logic from downstream projects.

If we use e.g. localName: Object we should change to localName: EmberObject so that downstream projects don't have that problem, but I don't see how that relates to making localName a required key

So you are suggesting to only have localName set when it would differ from export? I think that is fine, it just means that all downstream consumers will have to handle the mapping.localName || mapping.export logic themselves if they need to come up with a local name.

@Turbo87
Copy link
Member

Turbo87 commented Apr 24, 2018

So you are suggesting to only have localName set when it would differ from export?

yep, I think that was the logic until now, right?

@rwjblue
Copy link
Member Author

rwjblue commented Apr 24, 2018

yep, I think that was the logic until now, right?

We had a few other scenarios (e.g. #45) where we have customized the localName other than just reserved words. Another example is emberjs/rfcs#273 (comment), where we would have a localName of injectService for import { inject } from '@ember/service'.

However, as long as the guidance for downstream projects is to always do m.localName || m.export to decide a suggested local name, we have no issues with leaving localName out when it wouldn't differ from export.

@rwjblue rwjblue force-pushed the flesh-out-formatting branch from 3160ba8 to fe46df7 Compare April 24, 2018 13:10
mappings.json Outdated
@@ -2404,6 +2404,7 @@
"global": "Ember.RSVP.Promise",
"module": "rsvp",
"export": "Promise",
"localName": "EmberPromise",
Copy link
Member

@Turbo87 Turbo87 Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure about this? I've always used Promise instead, because I never wanted the native Promise and it's essentially a polyfill too 🤔

and if we want to use a different name, why not RSVPPromise or RSVPromise? 😉

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. I agree, I want to name it Promise for exactly the same reason.

@rwjblue
Copy link
Member Author

rwjblue commented Apr 24, 2018

Updated to remove localName when it is the same as export, were the rest of the changes reasonable?

@Turbo87
Copy link
Member

Turbo87 commented Apr 24, 2018

see above :P

@rwjblue rwjblue merged commit 9902d62 into ember-cli:master Apr 24, 2018
@rwjblue rwjblue deleted the flesh-out-formatting branch April 24, 2018 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants