Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Jan 30, 2023

At build time, a warning is logged:

Usages of the Ember Global may be caused by an outdated ember-cli-babel
dependency. The following steps may help:

* Upgrade the following addons to the latest version:
  * ember-styleguide

As such, let's bump to the newest version of this package. See https://github.com/ember-learn/ember-styleguide/releases for the package's release notes.

This should help along the way to achieving: #1272.

@netlify
Copy link

netlify bot commented Jan 30, 2023

Deploy Preview for ember-deprecations ready!

Name Link
🔨 Latest commit 410ea22
🔍 Latest deploy log https://app.netlify.com/sites/ember-deprecations/deploys/63dc7b339694fa0008185859
😎 Deploy Preview https://deploy-preview-1279--ember-deprecations.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

At build time, a warning is logged:
```
Usages of the Ember Global may be caused by an outdated ember-cli-babel
dependency. The following steps may help:

* Upgrade the following addons to the latest version:
  * ember-styleguide
```

As such, let's bump to the newest version of this package. See
https://github.com/ember-learn/ember-styleguide/releases for the
package's release notes.

This should help along the way to achieving: #1272.
@jenweber
Copy link
Contributor

Removing the octane line from .template-lintrc.js should fix the failing test.

The failing linter command was npm run lint:hbs. I'm not sure why this PR's change in particular makes us hit that. I would have expected it to fail when I upgraded the template lint dependency itself, not here. Anyway, the config option for Octane was removed in ember-template-lint v4.

@ghost
Copy link
Author

ghost commented Jan 30, 2023

Ah, sorry about that!

I'll take a look tonight and see if I can get a working change set out for your review.

Let's update our dependency on `ember-showdown-prism` to v3.2.0. This
will ensure that all of our dependencies' dependencies on
`@ember/render-modifiers` can be resolved to a single version.
For context, `ember/dependency-lint` is an addon that adds lint tests
that verify only one version of any given addon will be activated in the
final built application.

Right now, we've explicitly overriden that behavior for
`ember-concurrency` in the config/dependency-lint.js file to allow
either version 2.1.2 or version 1.3.0 to be installed. Given this
configuration, if any other version is installed, ember/dependency-lint
will throw a linter error.

After updating `ember-styleguide` we only depend on v2.3.7 of the
ember-concurrency package, so no extra configuration should be required
for this package to lint successfully. As such, let's go ahead and
remove this setting in the config file.
@ghost ghost changed the title package: update ember-styleguide dep package: update ember-styleguide, ember-showdown-prism deps Jan 31, 2023
@ghost
Copy link
Author

ghost commented Jan 31, 2023

Hi @jenweber, I think that the linter issues are coming from ember deprecation-lint. At a high level, it looks like there are really two separate problems here:

  • We (and our dependencies) depend on different incompatible versions of @ember/render-modifiers
  • Our deprecation-lint config setting for the allowed versions of @ember-concurrency does not include the version that we and ember-styleguide both want to use.

Digging into the first issue a bit, the dependency tree looks like:

deprecation-app

├── @ember/[email protected]
├─┬ ember-showdown-prism
│ └─┬ ember-prism
│   └── @ember/[email protected]
└─┬ ember-styleguide
  └── @ember/[email protected]

Interestingly, we do depend on render-modifiers, but don't appear to actually use it from what I can tell, so I submitted #1283 to remove that. For ember-showdown-prism, I went ahead and updated that in this pull request since it needs to be updated in lockstep with ember-styleguide. edit; Additionally, ember-showdown-prism depends on ember-auto-import v2 or greater, so I've pulled that in too.

For the second issue, since we and ember-styleguide want to use the same version, I think that we should just be able to remove that allowedVersion setting in the config/dependency-lint.js file to resolve the linter error.

I think that once #1283 has been merged, this should lint successfully (or at least it did when I merged it locally).

Please let me know if there's anything else that I can do here! 😄

`ember-prism` requires `ember-auto-import` >= v2, and without it, the
app will not build successfully. As such, let's update our dependency to
be compatible.
@ghost ghost changed the title package: update ember-styleguide, ember-showdown-prism deps package: update ember-styleguide, ember-showdown-prism, ember-auto-import deps Jan 31, 2023
@jenweber
Copy link
Contributor

jenweber commented Feb 3, 2023

I updated this branch with main after merging #1283. The tests are still unhappy, but this time we get a different error output:

this version of ember-auto-import requires the app to have a dependency on webpack 5

I think possibly this can be solved by installing webpack. It looks like they are still on v5.

npm install --save-dev webpack

@ghost
Copy link
Author

ghost commented Feb 3, 2023

Ah, darn! I'm not sure why I'm not seeing that locally... I'll try your suggestion and see if that makes the CI happy. Sorry for the inconvenience here.

Let's install a dev dependency on v5 of webpack in an attempt to resolve the
error:

```
[ember-auto-import] this version of ember-auto-import requires the app to have
a dependency on webpack 5
```

which occurs when running the test script via CI.
@ghost
Copy link
Author

ghost commented Feb 3, 2023

Thanks for the suggestion, that seems to have done the trick!

Please let me know if there's anything else that we should do here. 😃

@ghost ghost mentioned this pull request Feb 3, 2023
Copy link
Contributor

@jenweber jenweber left a comment

Choose a reason for hiding this comment

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

Everything looks great in the click test, thank you!

@jenweber jenweber merged commit 616806d into ember-learn:main Feb 3, 2023
@ghost ghost deleted the update-ember-styleguide branch February 3, 2023 15:17
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