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

Convert to V2 addon #411

Merged
merged 4 commits into from
May 11, 2023

Conversation

herzzanu
Copy link
Collaborator

No description provided.

@herzzanu herzzanu force-pushed the convert-to-addon-V2 branch 9 times, most recently from 0ca168c to bf02610 Compare March 16, 2023 23:22
@herzzanu herzzanu self-assigned this Mar 16, 2023
@herzzanu herzzanu force-pushed the convert-to-addon-V2 branch from bf02610 to 8ce8d1b Compare March 16, 2023 23:38
@herzzanu herzzanu requested a review from achambers March 16, 2023 23:38
@herzzanu herzzanu force-pushed the convert-to-addon-V2 branch 5 times, most recently from 9e98b01 to f6d56df Compare March 18, 2023 01:51
@SkoebaSteve
Copy link
Collaborator

@herzzanu ok to wait with merging this until we release v2?

@herzzanu
Copy link
Collaborator Author

@SkoebaSteve are we good to merge at this point or waiting for something else? Happy to help and unblock the situation.

@@ -46,7 +50,7 @@ jobs:
- name: Install Dependencies
run: yarn install --frozen-lockfile
- name: Run Tests
run: yarn test:ember
run: yarn test

This comment was marked as outdated.

@@ -77,4 +80,5 @@ jobs:
- name: Install Dependencies
run: yarn install --frozen-lockfile
- name: Run Tests
run: ./node_modules/.bin/ember try:one ${{ matrix.try-scenario }}
run: yarn ember try:one ${{ matrix.try-scenario }}
working-directory: ./test-app
Copy link

@anas7asia anas7asia Apr 26, 2023

Choose a reason for hiding this comment

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

Suggested change
working-directory: ./test-app
working-directory: test-app

We can make it even a bit shorter 😄

@anas7asia
Copy link

Maybe we can add a prettier configuration for the addon as well?

@@ -0,0 +1,9 @@
The MIT License (MIT)

Choose a reason for hiding this comment

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

This file can be left on the root level, it is copied by rollup when publishing

Choose a reason for hiding this comment

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

README can be left on the root level as well, it is copied by rollup when publishing:

    // Copy Readme and License into published package
    copy({
      targets: [
        { src: '../README.md', dest: '.' },
        { src: '../LICENSE.md', dest: '.' },
      ],
    }),

// please specify an object with the list of modules as keys
// along with the exports of each module as its value.

return app.toTree();

Choose a reason for hiding this comment

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

Should we keep maybeEmbroider?

  const { maybeEmbroider } = require('@embroider/test-setup');
  return maybeEmbroider(app, {
    skipBabel: [
      {
        package: 'qunit',
      },
    ],
  });

@@ -0,0 +1,79 @@
{
"name": "ember-launch-darkly",
"version": "2.2.0",

Choose a reason for hiding this comment

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

Isn't it v3.0.0 now?

},
"dependencies": {
"@embroider/addon-shim": "^1.0.0",
"launchdarkly-js-client-sdk": "^2.24.2",

Choose a reason for hiding this comment

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

It was upgraded to ^3.1.1

"@rollup/plugin-babel": "^6.0.3",
"babel-eslint": "^10.1.0",
"concurrently": "^7.6.0",
"ember-source": "4.11.0",

Choose a reason for hiding this comment

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

@herzzanu
Copy link
Collaborator Author

herzzanu commented May 5, 2023

Maybe we can add a prettier configuration for the addon as well?

That's a great idea. I prefer to add it in a separate PR, once we merge, since it was not there before.

plugins: [
// These are the modules that users should be able to import from your
// addon. Anything not listed here may get optimized away.
addon.publicEntrypoints(['**/*.js']),

This comment was marked as outdated.

// "app" tree. Things in here should also be in publicEntrypoints above, but
// not everything in publicEntrypoints necessarily needs to go here.
addon.appReexports(['helpers/**/*.js']),

This comment was marked as resolved.

package.json Show resolved Hide resolved
@@ -0,0 +1,19 @@
# EditorConfig helps developers define and maintain consistent

Choose a reason for hiding this comment

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

Should it stay at the root level? 🤔

@@ -0,0 +1,12 @@
'use strict';

module.exports = {

This comment was marked as resolved.


* `git clone <repository-url>` this repository
* `cd test-app`
* `npm install`

Choose a reason for hiding this comment

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

Suggested change
* `npm install`
* `yarn install`

Comment on lines 38 to 39
* `npm run lint`
* `npm run lint:fix`

Choose a reason for hiding this comment

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

Suggested change
* `npm run lint`
* `npm run lint:fix`
* `yarn lint`
* `yarn lint:fix`


import { variation } from 'ember-launch-darkly';

export default class IdentifiedUserController extends Controller {
get shape() {
return capitalize(variation('shape'));
return variation('shape');

This comment was marked as resolved.

<br />
<br />

{{#let (component (ensure-safe-component this.shape)) as |Shape|}}

This comment was marked as resolved.

"--yarn",
"--no-welcome"
"--no-welcome",
"--ci-provider=github"

Choose a reason for hiding this comment

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

Suggested change
"--ci-provider=github"
"--yarn"

@@ -23,18 +23,8 @@ module.exports = async function () {
},
},
},
{
name: 'ember-lts-4.4',

This comment was marked as resolved.

edition: 'classic',
},
}),
embroiderOptimized({

This comment was marked as resolved.

"description": "Small description for test-app goes here",
"repository": "",
"license": "MIT",
"author": "",

This comment was marked as resolved.

@@ -11,9 +11,7 @@ module.exports = {
ci: [
// --no-sandbox is needed when running Chrome inside a container
process.env.CI ? '--no-sandbox' : null,
process.platform === 'darwin' ? '--crash-dumps-dir=/tmp' : null, // Fix for https://superuser.com/questions/1292863/chrome-crashpad-crashes-on-xattr

This comment was marked as outdated.

@herzzanu herzzanu force-pushed the convert-to-addon-V2 branch 2 times, most recently from ea2a036 to 31f7e9a Compare May 10, 2023 13:23
"./-sdk/initialize": "./dist/-sdk/initialize.js",
"./-sdk/variation": "./dist/-sdk/variation.js",
"./addon-main.js": "./addon-main.js"
}

This comment was marked as resolved.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we can keep them Added.

Choose a reason for hiding this comment

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

Alright, thanks!

@herzzanu herzzanu force-pushed the convert-to-addon-V2 branch from 31f7e9a to 6a57006 Compare May 10, 2023 13:52
@herzzanu herzzanu merged commit 4d679a2 into adopted-ember-addons:master May 11, 2023
@herzzanu herzzanu deleted the convert-to-addon-V2 branch May 11, 2023 10:35
@tniezurawski
Copy link

@herzzanu This might be a bit odd question but I wanted to help the community to move some addons from V1 to V2 so... how did you know what to do? 😅 Is there some "v1 to v2 TODO list" resource?

@herzzanu
Copy link
Collaborator Author

@herzzanu This might be a bit odd question, but I wanted to help the community to move some addons from V1 to V2 so... how did you know what to do? 😅 Is there some "v1 to v2 TODO list" resource?

@tniezurawski, that's great to hear 🙌 Here's a non-exhaustive list of resources that could help:

Guides

Tools

A couple of v2 addons that could serve as inspiration

The list of V2 add-ons can go on. It's also a good idea to find the specific commits that helped migrate these addons to understand the nature of the changes. Hope it helps. Happy to help if you need anything specific on any of the projects you plan to work on.

@tniezurawski
Copy link

Great stuff @herzzanu! Thank you!

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