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 #576

Merged
merged 6 commits into from
Jul 30, 2022
Merged

Convert to v2 addon #576

merged 6 commits into from
Jul 30, 2022

Conversation

SergeAstapov
Copy link
Contributor

Follows Porting an Addon to V2 guide

with:
node-version: 12.x
cache: npm

- name: npm8
run: npm install -g npm@8
Copy link
Contributor Author

Choose a reason for hiding this comment

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

npm@8 is needed for workspaces support

@@ -1,19 +0,0 @@
Copyright (c) 2015-2016 Santiago Ferreira
Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted in favor of LICENSE.md

* Ember.js v3.16 or above
* Ember CLI v2.13 or above
* Node.js v10 or above
* Node.js v12 or above
Copy link
Contributor Author

Choose a reason for hiding this comment

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

engines field already requires v12, hence it's not technically dropping support for Node.js v10 but a correction in readme


module.exports = {
root: true,
parser: '@babel/eslint-parser',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is for ESLint v8 support

var setupTestHooks = blueprintHelpers.setupTestHooks;
var emberNew = blueprintHelpers.emberNew;
var emberGenerateDestroy = blueprintHelpers.emberGenerateDestroy;
let blueprintHelpers = require('ember-cli-blueprint-test-helpers/helpers');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes in this files made to make prettier happy

@@ -1,5 +1,5 @@
import { $, buildSelector, findClosestValue, guardMultiple } from './helpers';
import { getAdapter } from '../adapters';
import { getAdapter } from '../adapters/index';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rollup wasn't happy with previous path :/


let jQuery;

if (macroCondition(dependencySatisfies('@ember/jquery', '*'))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is to use jquery shipped by @ember/jquery if present, otherwise includes own jquery

@@ -1,5 +1,4 @@
import { resolve } from 'rsvp';
import { getAdapter } from '../adapters';
import { getAdapter } from '../adapters/index';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this changed to make rollup happy and rsvp removed as ember-source package ships rsvp bundled in hence we can't ship own copy.
Luckily, we could easily switch to native promise

"private": true,
"description": "Docs app for ember-cli-page-object",
"scripts": {
"build": "node ./index.js"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now docs/guides completely separated from the addon and the test addon

@@ -2,12 +2,17 @@

module.exports = {
root: true,
parser: '@babel/eslint-parser',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this allows using ESLint v8

@ro0gr
Copy link
Collaborator

ro0gr commented May 16, 2022

Thank you very much for this work! I'll do my best to check it this weekend.

@ro0gr
Copy link
Collaborator

ro0gr commented Jun 13, 2022

Overall looks fantastic!

Me, as a not an active monorepo user, is mostly interested in how to release the thing? At the moment we have the following release instructions https:/san650/ember-cli-page-object/wiki/How-to-release-a-new-version.

May you provide steps to release the /addon with /docs, so I can figure out how to update documentation, please?

@NullVoxPopuli
Copy link
Contributor

any chance we can get this released -- I'm trying to upgrade to ember 4 / deal with ember 3.28 deprecations and ember-cli-page-object is my last blocker


@ro0gr , re: releasing

the addon folder with "just" be npm publish (after cding into /addon)

for docs, looks like you use this: https:/san650/ember-cli-page-object/blob/master/docs.js

we'd need to test it out, but I think you'd move that file into the docs folder and just invoke it.
oh! this needs updating: https:/san650/ember-cli-page-object/blob/master/docs.js#L264

@NullVoxPopuli
Copy link
Contributor

@SergeAstapov is this a non-breaking change? (other than now requiring ember-auto-import@v2?)

@SergeAstapov
Copy link
Contributor Author

@ro0gr I'm sorry, missed you question.

TL;DR I will create a PR adding release-it config and adding RELEASE.md with release instructions (which would be very simple).

Longer answer.
I would go with release-it for release automation: it is pretty simple to setup (e.g. you can see PR in ember-click-outside-modifier as an example) and we can setup docs deploy via the hook (like it's done in ember-keyboard).

As a pros, it automatically publishes to npm, creates GitHub release, generates changelog and monorepo setup is super simple (thanks to release-it-yarn-workspaces plugin)

The only downside (in my opinion) - it requires labels to be added to pull requests.

semantic-release or standard-version could be another option, however to me they are not that straightforward and easy to use as release-it and they require diligence in commit messages (which hard to change retrospectively compared to PR labels)


@NullVoxPopuli correct, no breaking changes except ember-auto-import@v2 (which *is* a breaking change).

docs/index.js Outdated Show resolved Hide resolved
@ro0gr
Copy link
Collaborator

ro0gr commented Jul 7, 2022

@SergeAstapov thank you for the update!

I do really like an idea of release automation. However, I don't want to block this PR with it, since it's already a great move! I think for the time being we can live with a manual release process, while we have it clearly defined. And we can deliver release automation separately.

My question for now is how do we intend to release the whole thing? To me it looks like it's fine just release the /addon dir. If such, I believe we should update the docs generator script(#576 (comment)).

What do you think about the preferred release strategy, if leave an exact release tool out of the equation for now?

@SergeAstapov
Copy link
Contributor Author

@ro0gr sure, totally makes sense to keep release process as is and I'll submit follow up PR where we can work on automation separately.

We just should make following changes in https:/san650/ember-cli-page-object/wiki/How-to-release-a-new-version:

  • make sure both npm version and npm publish run from addon folder.
  • replace npm run docs command with cd ../docs && npm run build

Rest of the steps don't seem to change.

@ro0gr
Copy link
Collaborator

ro0gr commented Jul 19, 2022

Hey @SergeAstapov, thanks for the update!

I'm trying to figure out CI failure for the test scenarios w/o a lock file:

not ok 1 Chrome 103.0 - [undefined ms] - Global error: Uncaught Error: Could not find module @glimmer/manager imported from (require) at http://localhost:7357/assets/vendor.js, line 259

I think this is first time I see this kind of error. The current v2-beta branch does not have this failure(see #578). Do you have an idea which change can cause this?

@ro0gr
Copy link
Collaborator

ro0gr commented Jul 19, 2022

Alright, seems like downgrading to the "@ember/test-helpers": "~2.7.0" does the trick. For some reason we end up in the https:/emberjs/ember-test-helpers/blob/2b1ddcc91a971e1a3e1509d55f711778c32984b1/addon-test-support/%40ember/test-helpers/-internal/get-component-manager.ts#L14-L17, which does not make sense to me and might mean some underlying bug there. I'll need some time to wrap my head around this issue.

Meanwhile feel free to add a commit to downgrade the test helpers if you want for now. Anyways, I'll do my best to finally cut the release this week.

"ember-cli-doc-server": "1.1.0",
"ember-cli-htmlbars": "^6.0.1",
"ember-cli-inject-live-reload": "^1.8.2",
"ember-cli-page-object": "2.0.0-beta.3",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've just noticed this, and I wonder if this is supposed to be a relative path, or use * instead of the hardcoded version?

@ro0gr ro0gr mentioned this pull request Jul 22, 2022
@ro0gr
Copy link
Collaborator

ro0gr commented Jul 30, 2022

Well, I figured out test failures and fixed it by migrating to pnpm #579

@ro0gr ro0gr merged commit 05390f2 into san650:v2-beta Jul 30, 2022
@ro0gr
Copy link
Collaborator

ro0gr commented Jul 30, 2022

@SergeAstapov thank you! 🚀

@NullVoxPopuli
Copy link
Contributor

fixed it by migrating to pnpm

Yet another situation where pnpm saves the day!

@ro0gr
Copy link
Collaborator

ro0gr commented Jul 31, 2022

just published it as 2.0.0-beta.4

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.

3 participants