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

Upgrade Ember.js from v3.28.3 to v4.10.0 #103

Merged
merged 4 commits into from
Apr 28, 2023

Conversation

MrChocolatine
Copy link
Member

@MrChocolatine MrChocolatine commented Feb 12, 2023

Build

Breaking: Upgrade to Ember.js version 4.10 (#103)

Requirements bumped to Ember.js/CLI version 3.28 or above.

Close #100 , #101, #102 .

@MrChocolatine MrChocolatine added dependencies Pull requests that update a dependency file internal Addon internal implem, no impact for users breaking Breaking changes labels Feb 12, 2023
@MrChocolatine MrChocolatine requested a review from a team as a code owner February 12, 2023 12:56
- Standardise helper's function signature and body
  To match what is generated by default when creating a new helper.

- Fix types errors provided by implicit TypeScript checks
  This will help whenever the project is migrated to TS
What was done and why:

- After the `stream` module, the modules `util` and `process` had to be
  added to the dependencies as well in order to satisfy Webpack v5 which
  does not include polyfills anymore, otherwise tests were failing.
  - https://webpack.js.org/migrate/5/
  - https://webpack.js.org/configuration/resolve/#resolvefallback
  - https://github.com/ef4/ember-auto-import/blob/v2.6.0/docs/upgrade-guide-2.0.md
  - https://gist.github.com/ef4/d2cf5672a93cf241fd47c020b9b3066a

  For the `process` module, adding it to the `fallback` key was not
  working, it had to be included in the `plugins` key:
  - https://stackoverflow.com/questions/65018431/webpack-5-uncaught-referenceerror-process-is-not-defined/65018686#65018686
  - https://discord.com/channels/480462759797063690/898671957007011841/898682272847384596
    Overall, these messages on Discord give the entire final solution in
    this commit.

- `wepack` moved to the dependencies because otherwise `eslint-plugin-n`
  was returning the error:

  ```
  "webpack" is not published.eslintn/no-unpublished-require
  ```

  See:
  - https://github.com/eslint-community/eslint-plugin-n/blob/15.6.1/docs/rules/no-unpublished-require.md
  - mysticatea/eslint-plugin-node#47
@BlueCutOfficial BlueCutOfficial force-pushed the upgrade-ember-to-lts-version-4.10 branch from 53338ed to 99cbf73 Compare March 15, 2023 10:14
@BlueCutOfficial
Copy link
Member

@MrChocolatine you were almost there. I dropped the commits "remove ember-page-title" (the dummy app template uses it) and "rebuild yarn.lock" and then rebase the MR with master. I'll have a deeper look to your commit messages and all the refs you linked.

@BlueCutOfficial
Copy link
Member

BlueCutOfficial commented Mar 15, 2023

The tradeoff of having webpack as a dependency in an Ember addon is not quite clear to me, even if I understand why it was done here 🤔 Surprisingly, when I checkout this branch as it is locally, I still have 3 errors when I run yarn lint, including the webpack unpublished stuff.

EDIT: I no longer have the error locally, something was maybe out of date on my side.

@BlueCutOfficial BlueCutOfficial force-pushed the upgrade-ember-to-lts-version-4.10 branch from 94509cb to 99cbf73 Compare April 28, 2023 10:30
@BlueCutOfficial BlueCutOfficial merged commit d78b6d4 into master Apr 28, 2023
@BlueCutOfficial BlueCutOfficial deleted the upgrade-ember-to-lts-version-4.10 branch April 28, 2023 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking changes dependencies Pull requests that update a dependency file internal Addon internal implem, no impact for users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants