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

Remove decorators #94

Merged
merged 10 commits into from
Oct 11, 2018
Merged

Remove decorators #94

merged 10 commits into from
Oct 11, 2018

Conversation

mhluska
Copy link
Contributor

@mhluska mhluska commented Oct 3, 2018

I was having some issues in my app related to decorators. I realized this is the only dependency in my tree relying on them. I started to think that it's somewhat strange to require them in a library that others might use as a building block.

This also removes yarn, removes some unneeded dependencies and fixes an npm vulnerability (npm audit fix --force).

I realize this is a pretty controversial PR and should probably be split up into smaller chunks. I can do it if needed.

I made sure tests are passing locally and I'm using this in production.

@artzte
Copy link

artzte commented Oct 4, 2018

+1 on merging this if possible. This patch unblocked us. Thanks @mhluska

@pzuraq
Copy link
Collaborator

pzuraq commented Oct 4, 2018

The referenced issue was actually related to a build time bug, which affected multiple projects in the Ember ecosystem and has been patched. You should be able to fix the issue by running yarn update or the equivalent to update your lockfiles.

Decorators have been stable for some time, and native class syntax is the future direction for Ember as a whole. ember-popper was admittedly an early adopter here, but we gained valuable insight from the project and it helped us pave that path forward 😄 that said, if @kybishop would like to transition back to EmberObject until native classes become the standard, I don't have any issues with that myself. I would recommend against removing Yarn, however, since it is still much faster for CI runs than NPM in my own experience.

@artzte
Copy link

artzte commented Oct 4, 2018

We are using NPM on our project. I am not seeing an update in our package-lock.json when clean-reinstalling our project dependencies, even after clearing my local cache. What do you think we are missing?

@pzuraq
Copy link
Collaborator

pzuraq commented Oct 4, 2018

I'm not too sure as I don't have much experience with NPM lockfiles. You should be able to check to see if the build dependency has updated though. The affected version was ember-compatibility-helpers@v1.1.0, and the fixed versions are ember-compatibility-helpers@v1.1.1 and ember-compatibility-helpers@v1.1.2. If you're still seeing the issue with either of those package versions, we should reopen an issue on ember-compatibility-helpers.

@mhluska
Copy link
Contributor Author

mhluska commented Oct 6, 2018

@pzuraq I'm totally fine with keeping yarn. I've reverted those changes.

@kybishop
Copy link
Owner

kybishop commented Oct 9, 2018

Sorry for the delay life has been absurdly busy lately.

I'm generally 👍 on this change. Exploring decorators has been interesting, but feels like it has caused more issues than it has solved. In the long-term, I'm much more interested in using Typescript over decorators.

I want to give this a slightly more thorough lookover in the next few days before merging.

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.

None yet

4 participants