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

Move isEnabled intention to included hook; fix ember-auto-import conflicts #434

Merged

Conversation

gfmartinez
Copy link
Contributor

@gfmartinez gfmartinez commented Mar 4, 2021

In reference to embroider-build/ember-auto-import#340
When using this addon, fastboot breaks with ember-auto-import past v 1.6.0 because the isEnabled hook resolves to false.

I've tried to keep the spirit/intent of the former use of the isEnabled hook and just block the file sync in the case of a production environment, since this addon and its dependencies are not needed in the production environment.

I've tested:

Locally
All our fastboot tests using ember-cli-fastboot-testing run and pass just as before.

Deployed
Previous to this change, an update to ember-auto-import past v1.6.0 broke and would error on the server. With this change we can update to v1.10.1 and have no issues.

Would love to discuss and have some additional folks test for their environments to see if this works well.

Additionally
Should consider updating ember-auto-import. Current version is at 1.2.15 and current is 1.10.1

@ryanto
Copy link
Member

ryanto commented Mar 8, 2021

Thanks for fixing this!

@ryanto ryanto merged commit 08b54ef into embermap:master Mar 8, 2021
@ryanto
Copy link
Member

ryanto commented Mar 8, 2021

Hey @gfmartinez! I tried to release this, but it didn't work because the ember-try scenarios are failing. The usual release process is to run the ember try:each command and make sure all LTS versions pass (versions like 2.18 can be skipped since they're no longer supported).

I have a hunch that this addon's dependencies are out of date, which is what's causing the ember-try builds to fail. My next step for diagnosing this would be to try to upgrade the dependencies to their latest versions.

Unfortunately I'm not actively maintaining this project anymore, so I won't be able to dive into it myself. There was some talk in issue #396 about some other folks taking over maintenance, so I think the best chance for getting this into a proper release would be to reach out to them or take a peek at it yourself.

Sorry this came up, but happy to answer any questions that pop up!

@gfmartinez
Copy link
Contributor Author

@ryanto Thanks for the info. Just looking again at this and it looks like my PR failed the embery-try:2.18 in CI.
If support for 2.18 is no longer viable, that's great: I can see getting rid of that test.

If it were all passing, it would most likely be ok to release, but this commit: ba1fcf0 got rid of the release script.
Were you going to release manually?

There are many issues with running ember try:each locally that I've run into in the past. Most of them have to do with versioning the env. Like which node and yarn versions are running for the tests, etc. It's probably more reliable to count on the CI setup to run that. I mean, can you run ember try:each and have the tests pass locally on a recent previous branch? I tried a few with my local current node and yarn versions, as well as setting to the CI env versions of 10.24.0 and 1.3.2 respectively. Both quite out of date.

When I run ember try:each locally, I get a failure on my PR and recent commits. All errors come from the use of the nullish coalescing operator in the ember-modal-dialog dependency. This should be an easy fix via updating babel. It would take some work to get it to all go together.

Probably the easiest way forward, before a major overhaul and update of all dependencies would be to remove the 2.18 trial in the travis.yml (as you've done), and rely on CI for deploy. My PR passed all the version trials except for 2.18 for an unrelated issue in ember-source v2.18.2.

I'm happy to work with you to get this releasable and released so we can solve our issues and those of the other that have run into the same issue. How can we best move forward?

Thanks again for all your effort!

@ryanto
Copy link
Member

ryanto commented Mar 8, 2021

I think the reason CI is passing is because it has its dependencies cached. When you run this locally you get fresh dependencies and the build fails.

I removed the release script from travis because I wanted to do it all locally (given that travis takes hours to run these days!).

However, since travis can correctly build and release let's add it back.

If you PR...

  • The release script back to .travis.yml
  • Remove 2.18 so travis tests pass

Then I can merge and tag, and that should get us a release.

@gfmartinez
Copy link
Contributor Author

That works for me. I can do that tomorrow morning.
Then later this week I'll have time to do some updates to get the tests to pass locally with some updates.
Thanks for working with me on it.

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.

2 participants