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

BREAKING Migrate to V2 Addon #258

Merged
merged 9 commits into from
Apr 27, 2023
Merged

BREAKING Migrate to V2 Addon #258

merged 9 commits into from
Apr 27, 2023

Conversation

BlueCutOfficial
Copy link
Member

@BlueCutOfficial BlueCutOfficial commented Feb 24, 2023

Breaking

Migrate to V2 Addon

Follow the guide https://github.com/embroider-build/embroider/blob/main/PORTING-ADDONS-TO-V2.md to migrate ember-slugify to V2 Addon.


See below various troubleshooting that could be handy for other addons:

.mjs extension for rollup.config

⚠️ By following the doc PORTING-ADDONS-TO-V2 as it is, the addon built ran into:

RollupError: Node tried to load your configuration file as CommonJS even though it is likely an ES module. To resolve this, change the extension of your configuration to ".mjs", set "type": "module" in your package.json file or pass the "--bundleConfigAsCjs" flag.

Based on ember-welcome-page structure, rollup.config.js was replaced with rollup.config.mjs. It solved the issue.

ember-try 3.0.0-beta.1

With ember-try 2.0.0, running yarn test:ember-compatibility triggered the following error coming from fs-extra:

Source and destination must not be the same

This bug was fixed in ember-try 3.0.0-beta.1 with PR ember-try#912 that downgrades fs-extra.

Build before testing

The test-app relies on the addon build. Installing the dependencies is not enough to make the tests run, you also need to build the addon first:

# ci.yml

- name: Build Addon
  run: yarn build

- name: Run Tests
  run: yarn test

Top-level package.json:

"scripts": {
    "build": "yarn workspace ember-slugify run build",
    "test": "yarn workspace test-app test"
 }

@BlueCutOfficial BlueCutOfficial changed the title BREAKING split addon and test-app folders into a monorepo BREAKING Migrate to V2 Addon Feb 27, 2023
@BlueCutOfficial BlueCutOfficial force-pushed the migration-addon-v2 branch 7 times, most recently from 0c87df5 to 9b639a8 Compare March 3, 2023 11:26
@BlueCutOfficial BlueCutOfficial marked this pull request as ready for review March 7, 2023 15:25
@BlueCutOfficial BlueCutOfficial requested a review from a team as a code owner March 7, 2023 15:25
@GreatWizard
Copy link
Member

GreatWizard commented Mar 12, 2023

@@ -36,13 +36,13 @@ module.exports = {
},
},
{
files: ['**/tests/dummy/**/*.hbs', '**/tests/**/*.{js,ts}'],
files: ['**/*.hbs', '**/tests/**/*.{js,ts}'],
Copy link
Member

Choose a reason for hiding this comment

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

Running yarn lint with this file content works, I believe other rules/overrides are not needed in test-app directory (they are only needed for files in ./ember-slugify/src/*)

'use strict'

module.exports = {
  extends: ['recommended', 'stylistic'],

  overrides: [
    {
      files: ['**/tests/**/*.{js,ts}'],
      rules: {
        'eol-last': 'off',
      },
    },
  ],
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey, I think I am going to merge the PR as it is once green because it really starts to hit hard on my mental health and I want to unblock the dependencies update. Let's have a better look at the lint config afterward :)

ember-slugify/package.json Show resolved Hide resolved
Comment on lines +1 to +7
{
"plugins": [
"@embroider/addon-dev/template-colocation-plugin",
["@babel/plugin-proposal-decorators", { "legacy": true }],
"@babel/plugin-proposal-class-properties"
]
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain why it is needed?

Copy link
Member Author

@BlueCutOfficial BlueCutOfficial Mar 24, 2023

Choose a reason for hiding this comment

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

I blindly followed the doc that says

Grab the example babel config and save it as addon/babel.config.json

😅

Copy link
Member

Choose a reason for hiding this comment

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

Could be nice to see if they are really needed on every addon or if we can remove them

@BlueCutOfficial BlueCutOfficial force-pushed the migration-addon-v2 branch 3 times, most recently from c7ae5a9 to f7045be Compare March 24, 2023 17:13
@BlueCutOfficial BlueCutOfficial force-pushed the migration-addon-v2 branch 2 times, most recently from 64880ad to af67dd5 Compare April 14, 2023 13:24
@BlueCutOfficial BlueCutOfficial force-pushed the migration-addon-v2 branch 3 times, most recently from 18d0d6b to e8131df Compare April 27, 2023 14:53
@BlueCutOfficial BlueCutOfficial merged commit 0ebf0b8 into master Apr 27, 2023
@BlueCutOfficial BlueCutOfficial deleted the migration-addon-v2 branch April 27, 2023 15:05
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.

4 participants