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

Feature/update libraries #45

Merged
merged 9 commits into from Feb 14, 2020
Merged

Conversation

mfeckie
Copy link
Contributor

@mfeckie mfeckie commented Oct 13, 2019

This PR

  • Upgrades to Ember 3.16
  • Upgrades to broccoli-autoprefixer 7.x
  • Addresses changes to autoprefixer API
  • Adds changelog entry to note breaking change

I noticed that the changleog suggests that broccoli-autoprefixer is already at 7, but this isn't reflected in package.json.

@snewcomer
Copy link
Collaborator

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 adopted-ember-addons?

@kimroen
Copy link
Owner

kimroen commented Feb 13, 2020

@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 ember-cli-document-title, so if you can find the time and attention to give to these projects, please feel free to do so, and let's talk directly about access to publishing once there is something to publish.

Transferring over to adopted-ember-addons also feels like a good idea.

Thanks again!

Copy link
Collaborator

@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.

@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?

@snewcomer
Copy link
Collaborator

@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. We can explore the adopted addons, but only with your explicit permission!

@mfeckie
Copy link
Contributor Author

mfeckie commented Feb 13, 2020

@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
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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!

@kimroen
Copy link
Owner

kimroen commented Feb 14, 2020

@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. We can explore the adopted addons, but only with your explicit permission!

@snewcomer You now have publish rights to both packages :)

@snewcomer snewcomer merged commit e637a66 into kimroen:master Feb 14, 2020
@snewcomer
Copy link
Collaborator

@mfeckie Added to 1.0.0-beta.0. Our app (as is all of them :)) is very large and we have some CSS tweaks to work out it looks like as I tested this. Other teams are verifying this so in the meantime put it in beta for a little bit until I get some feedback! Thanks again for everything.

This was referenced Feb 14, 2020
@mfeckie mfeckie deleted the feature/update-libraries branch February 15, 2020 02:45
@bradleypriest
Copy link

This PR seems to have dropped compatibility with EmberCLI config/targets.js was that intentional, or an oversight?

@mfeckie
Copy link
Contributor Author

mfeckie commented Apr 1, 2020

It was intentional to bring the library in line with how autoprefixer expects things to be.

@bradleypriest
Copy link

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.

@mfeckie
Copy link
Contributor Author

mfeckie commented Apr 1, 2020

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

@bradleypriest bradleypriest mentioned this pull request May 1, 2020
@snewcomer
Copy link
Collaborator

ref #51 To add back support. Lmk what you think!

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

4 participants