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

Port code to ember-rfc176-data new format #142

Merged
merged 12 commits into from
May 15, 2018

Conversation

Serabe
Copy link
Contributor

@Serabe Serabe commented Sep 18, 2017

This PR adapts the code to the new format in
ember-cli/ember-rfc176-data#37

This also removes a line testing an old shims not ported to new format.

@Turbo87
Copy link
Member

Turbo87 commented Sep 19, 2017

This also removes a line testing an old shims not ported to new format.

this is a problem, since we should still warn about this even if there is no equivalent in the new mappings... 🤔

@rwjblue
Copy link
Member

rwjblue commented Sep 19, 2017

Ya, we should explicitly support having deprecated: true without a replacement value.

@Serabe
Copy link
Contributor Author

Serabe commented Sep 19, 2017

Then either the old-shims file should not be removed or the data that was lost in translation brought back.

@rwjblue
Copy link
Member

rwjblue commented Sep 19, 2017

Hmm, not sure I'm following. All of the old shims data is included in the new mappings.json file isn't it?

@rwjblue
Copy link
Member

rwjblue commented Sep 19, 2017

Chatted with @Serabe in slack about this, and summarized in ember-cli/ember-rfc176-data#37 (comment).

@Turbo87
Copy link
Member

Turbo87 commented Sep 19, 2017

we might also want to think about adding an fromOldShims: true to the relevant entries, otherwise at some point this rule will start warning about deprecated imports that are not actually part of the old shims

@Serabe
Copy link
Contributor Author

Serabe commented Sep 19, 2017

Right.

@rwjblue
Copy link
Member

rwjblue commented Sep 19, 2017

Ya, but I actually think a general rule for no-deprecated-modules is a better overall path forward anyways...

@Turbo87
Copy link
Member

Turbo87 commented Sep 20, 2017

Ya, but I actually think a general rule for no-deprecated-modules is a better overall path forward anyways...

I tend to disagree. The ember-cli-shims are deprecated independent of the Ember version that is used, but the other things might depend on the Ember version. In other words: if I'm on an old Ember version I do want to disallow the old shims, but I might not want to disallow legitimate API that is not yet deprecated in the Ember version that I'm using.

@rwjblue
Copy link
Member

rwjblue commented Sep 20, 2017

In other words: if I'm on an old Ember version I do want to disallow the old shims, but I might not want to disallow legitimate API that is not yet deprecated in the Ember version that I'm using.

Yep, totally agreed. At the moment we only have things that should have always worked, but I totally agree that at some future time we could deprecate a module in favor of some replacement that doesn't exist for certain Ember versions. This doesn't mean my suggested path forward is wrong 😉, it just means we need more data than deprecated: true.

For deprecations as of a specific Ember version, we should encode that into the updated mappings.json file format. I'm thinking deprecated: '1.2.3' (where its as of that specific version, and we can check Ember version against this string)...

@Turbo87
Copy link
Member

Turbo87 commented Sep 20, 2017

what we could do is have a static list of module paths in this plugin that were part of ember-cli-shims and only warn if we detect an import from such a module path. I'm not sure that information needs to live in the RFC data repo 🤔

@michalsnik
Copy link
Member

I agree with @Turbo87, but having a static list of modules used in ember-cli-shims will make it impossible to automatically fix those imports to recommended ones wouldn't it?

@Turbo87
Copy link
Member

Turbo87 commented Sep 22, 2017

but having a static list of modules used in ember-cli-shims will make it impossible to automatically fix those imports to recommended ones wouldn't it?

I don't understand why that would be a problem? 🤔

@Serabe Serabe force-pushed the feature/new-data-format branch 2 times, most recently from c3143e3 to fa1bc9f Compare September 29, 2017 13:26
@Serabe
Copy link
Contributor Author

Serabe commented Sep 29, 2017

I need to keep the force upgrade of ember-rfc176-data or the tests won't pass. I'll update once ember-rfc176-data has been updated.

@rwjblue
Copy link
Member

rwjblue commented Oct 3, 2017

Ready to update to point at [email protected] now that it is published.

@Serabe
Copy link
Contributor Author

Serabe commented Oct 3, 2017

@rwjblue done!

.travis.yml Outdated
@@ -8,6 +8,7 @@ node_js:
cache: yarn

script:
- yarn upgrade ember-rfc176-data
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be needed any more...

package.json Outdated
@@ -53,7 +53,7 @@
"jest": "^20.0.4"
},
"dependencies": {
"ember-rfc176-data": "^0.2.7",
"ember-rfc176-data": "~0.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

Should probably still be ^0.3.0 (it just has the same effect as ~0.3.0 in this case)

@rwjblue rwjblue force-pushed the feature/new-data-format branch from 8a12d8b to 5468e62 Compare May 14, 2018 20:00
@rwjblue
Copy link
Member

rwjblue commented May 14, 2018

I just updated with:

  • removing the no-deprecated-module rule (for now, but will make separate PR preserving authorship for that after this lands)
  • Fixing a few conflicts
  • Inline data for no-old-shims rule

Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

can you add a test for Ember.String.htmlSafe to check that we're handling the deprecated flag correctly?

@rwjblue
Copy link
Member

rwjblue commented May 14, 2018

can you add a test for Ember.String.htmlSafe to check that we're handling the deprecated flag correctly?

sure, also still working on a few of the remaining failures (seem related to destructuring)

@rwjblue
Copy link
Member

rwjblue commented May 15, 2018

can you add a test for Ember.String.htmlSafe to check that we're handling the deprecated flag correctly?

We already had a test for htmlSafe, I updated the expectation to the correct value...

@rwjblue rwjblue requested a review from Turbo87 May 15, 2018 20:24
@Turbo87
Copy link
Member

Turbo87 commented May 15, 2018

hmm, about the htmlSafe, do we eventually need to care about the ember-source version that is used?

@rwjblue rwjblue merged commit 098f865 into ember-cli:master May 15, 2018
@rwjblue
Copy link
Member

rwjblue commented May 15, 2018

hmm, about the htmlSafe, do we eventually need to care about the ember-source version that is used?

yes, but not now 😄, both import { htmlSafe } from '@ember/string' and import { htmlSafe } from '@ember/template' currently point to Ember.String.htmlSafe

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.

4 participants