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

Add new use-ember-data-rfc-395-imports rule #450

Merged
merged 11 commits into from Aug 8, 2019
Merged

Add new use-ember-data-rfc-395-imports rule #450

merged 11 commits into from Aug 8, 2019

Conversation

dcyriller
Copy link
Contributor

@dcyriller dcyriller commented Jul 11, 2019

Description

This PR introduces a new rule to ease ember-data migration to new packages: use-ember-data-rfc-395-imports.

Examples of incorrect code for this rule:

import Model from "ember-data/model";
import attr from "ember-data/attr";

import DS from "ember-data";
const { Model } = "ember-data";

Examples of correct code for this rule:

import Model, { attr } from "@ember-data/model";

import Model from "@ember-data/model";

Note

Split into 5 commits that may be reviewed one after the other.

It leverages what have been done in the past with Ember globals and ember-cli-shims.

A few discussion are still in progress around https://github.com/ember-data/ember-data-rfc395-data/ (the data set). Hence the WIP!

Links

ember-data RFC
a related codemod

EDIT: I've updated this description to reflect the changes made following ember-data/ember-data-rfc395-data#7

@dcyriller
Copy link
Contributor Author

cc @runspired @rwjblue

docs/rules/new-ember-data-packages.md Outdated Show resolved Hide resolved
lib/rules/new-ember-data-packages.js Outdated Show resolved Hide resolved
lib/rules/new-ember-data-packages.js Outdated Show resolved Hide resolved
lib/rules/new-module-imports.js Show resolved Hide resolved
lib/rules/new-module-imports.js Show resolved Hide resolved
tests/lib/rules/new-ember-data-packages.js Outdated Show resolved Hide resolved
tests/lib/rules/new-ember-data-packages.js Outdated Show resolved Hide resolved
tests/lib/rules/new-ember-data-packages.js Outdated Show resolved Hide resolved
tests/lib/rules/new-ember-data-packages.js Outdated Show resolved Hide resolved
tests/lib/rules/new-ember-data-packages.js Outdated Show resolved Hide resolved
@dcyriller dcyriller changed the title WIP: ember-data: Implement two rules to ease migration to the new packages WIP: ember-data: Add a new rule for @ember-data packages Jul 16, 2019
@dcyriller dcyriller changed the title WIP: ember-data: Add a new rule for @ember-data packages ember-data: Add a new rule for @ember-data packages Jul 19, 2019
@dcyriller
Copy link
Contributor Author

This is ready now.

Copy link

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

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

👍 Nice work!

docs/rules/new-ember-data-packages.md Outdated Show resolved Hide resolved
lib/rules/new-ember-data-packages.js Outdated Show resolved Hide resolved
lib/rules/new-ember-data-packages.js Outdated Show resolved Hide resolved
Copy link

@runspired runspired left a comment

Choose a reason for hiding this comment

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

I believe there should be a new entry for this rule in README.md and in recommended-rules.js (with a value of "off" for now). To achieve this use npm run update

lib/index.js Outdated Show resolved Hide resolved
lib/rules/new-ember-data-packages.js Outdated Show resolved Hide resolved
docs/rules/no-ember-data-global-imports.md Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
tests/lib/rules/no-ember-data-global-imports.js Outdated Show resolved Hide resolved
tests/lib/rules/no-ember-data-global-imports.js Outdated Show resolved Hide resolved
@bmish bmish changed the title ember-data: Add a new rule for @ember-data packages Add new no-ember-data-global-imports rule Aug 3, 2019
@rwjblue
Copy link
Member

rwjblue commented Aug 6, 2019

@bmish - Mind doing one more pass on this one before we land / release?

@rwjblue rwjblue requested a review from bmish August 6, 2019 14:04
@runspired
Copy link

I dislike being pedantic especially when I feel the name has been changed a few times, but is no-ember-data-global-imports really the best name for a rule that:

  • lints against older module import locations
  • AND lints against an older "single-export-object" style import
  • AND lints against global syntax

There was a suggestion to name the rule after the rfc, I think it's probably the way to go.

use-ember-data-rfc-395-imports

@dcyriller
Copy link
Contributor Author

There was a suggestion to name the rule after the rfc, I think it's probably the way to go. use-ember-data-rfc-395-imports

@rwjblue @bmish if you agree, I'll change that. Let me know! (I want to be sure everyone agrees before changing it.)

Copy link
Member

rwjblue commented Aug 6, 2019

Yeah, that seems fine to me. My only real objection was the "new" in the original name, I'm sorry for the run around / bike shedding here.

@dcyriller
Copy link
Contributor Author

No worries, I prefer to make it right!

@dcyriller dcyriller closed this Aug 6, 2019
@dcyriller dcyriller reopened this Aug 6, 2019
lib/utils/new-module.js Outdated Show resolved Hide resolved
lib/rules/use-ember-data-rfc-395-imports.js Outdated Show resolved Hide resolved
lib/utils/new-module.js Outdated Show resolved Hide resolved
@bmish bmish changed the title Add new no-ember-data-global-imports rule Add new use-ember-data-rfc-395-imports rule Aug 6, 2019
dcyriller and others added 10 commits August 6, 2019 23:45
..to `lib/utils/new-module`
It contains the mappings to migrate to new ember-data packages.
This rule aims at highlighting old ember-data imports. The goal is to
help users to migrate to new packages imports. New ember-data imports
are available from version `3.11`.

It prevents the following imports:
```
import DS from "ember-data;"
const { Model } = DS;
```
or
```
import Model from "ember-data/model";
```
Instead, these imports should be used:
```
import Model from "@ember-data/model";
```

This rule provides a fixer (or to be precise: this rule leverages the
`no-old-shims` fixer) for some cases. For the other cases, a codemod is
available.
@bmish
Copy link
Member

bmish commented Aug 6, 2019

@dcyriller don't worry, I'm not going to require you to refactor existing code if you're just moving it around. I commented about the excessive length and complexity of some of the functions because it makes the review difficult, and it could be a good time to clean this up. But we can merge the PR soon if you're done with your changes.

@dcyriller
Copy link
Contributor Author

@bmish should be good now.

@dcyriller
Copy link
Contributor Author

dcyriller commented Aug 7, 2019

I've made fun splitting utils functions into smaller ones (I really did 😀). I'm not 100% convinced it makes the code better though.

.travis.yml Show resolved Hide resolved
@bmish bmish merged commit 8d22073 into ember-cli:master Aug 8, 2019
@dcyriller dcyriller deleted the data-rfc395 branch August 8, 2019 17:23
@bendemboski
Copy link

bendemboski commented Aug 8, 2019

@dcyriller @bmish @rwjblue this caused a crashing regression. Let me know if there's any way I can help get this resolved as it's blocking me updating to the latest eslint-plugin-ember (and causing a number of my addon builds with floating dependencies to fail).

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.

None yet

6 participants