-
Notifications
You must be signed in to change notification settings - Fork 25
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
Feature/update libraries #45
Conversation
I really like this PR! @kimroen Do you have the bandwidth to review this PR. If not, what do you think about transferring over to |
@mfeckie Thank you so much for making this PR, and sorry for the radio silence. @snewcomer I also like it - thanks for poking me about it! Unfortunately, as we've talked about directly elsewhere I don't see myself having the capacity to look at this or my other ember projects now or in the near future. I've invited you (@snewcomer) to become a collaborator on this project and Transferring over to Thanks again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mfeckie This is certainly phenomenal work! I've tested and it works (saw we were using browserlist
so glad this caught it). It has been a few months. Do you have any other thoughts with this PR?
@kimroen Really really appreciate it! We are super grateful for these addons! If possible, could I get npm publish rights as well in the meantime? |
@snewcomer Have updated PR to bring it to Ember 3.16. No other changes. I'd be happy to see it merged, version bumped and released. Also happy to become a maintainer should it become adopted. |
|
||
# we recommend new addons test the current and previous LTS | ||
# as well as latest stable release (bonus points to beta/canary) | ||
- env: EMBER_TRY_SCENARIO=ember-lts-3.12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add 3.8 && even 3.4 for the time being? It is such a large bump and likely if users haven't explored different solution with autoprefixer, they might be on older version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added 3.8. 3.4 doesn't work, vendor.css
ends up empty.
I'm not sure it's reasonable to expect compatibility with 3.4. Could we make this a 1.x release and state that we support 3.8 and above? Folks on current versions can continue to use and folks that want updates (and are using 3.4) may need to do some work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that is totally fine! Was just ensuring we support the earliest versions that we could! Thanks a ton!
@snewcomer You now have publish rights to both packages :) |
@mfeckie Added to |
This PR seems to have dropped compatibility with EmberCLI |
It was intentional to bring the library in line with how autoprefixer expects things to be. |
In that case, it’s definitely worth adding a note in the changelog. And maybe a readme item on how to achieve the old logic if possible. I’d also wonder if you could reconsider? It definitely breaks the principle of least surprise for me that an ember-cli addon based around browser support doesn’t automatically hook up the existing browser support config that ember-cli provides. |
Happy to consider a PR with note for changelog and / or adding targets as an option again. We did make the release a major to allow for breaking changes |
ref #51 To add back support. Lmk what you think! |
This PR
I noticed that the changleog suggests that broccoli-autoprefixer is already at
7
, but this isn't reflected inpackage.json
.