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

Extract test app #1062

Merged
merged 5 commits into from
Jun 12, 2023
Merged

Extract test app #1062

merged 5 commits into from
Jun 12, 2023

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented May 4, 2023

Context, Plan, etc: https://gist.github.com/NullVoxPopuli/eafc7dad6547de5e730098498b829e1f

Due to errors discovered in the test-app extraction PR, here: #1061
I'm exploring using pnpm, in part because yarn does not handle peer dependencies correctly, and is not suitable for monorepos with strict resolution behavior.

Requires that these merged first:

Afterwards, the following are unblocked

  1. Convert to native package #1064

@NullVoxPopuli NullVoxPopuli changed the title Switch to pnpm Extract test app + Switch to pnpm May 4, 2023
@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review May 4, 2023 00:50
@ijlee2 ijlee2 mentioned this pull request May 4, 2023
Comment on lines +1 to +25
# unconventional js
/blueprints/*/files/
/vendor/

# compiled output
/dist/
/tmp/

# dependencies
/bower_components/
/node_modules/

# misc
/coverage/
!.*
.*/
.eslintcache

# ember-try
/.node_modules.ember-try/
/bower.json.ember-try
/npm-shrinkwrap.json.ember-try
/package.json.ember-try
/package-lock.json.ember-try
/yarn.lock.ember-try
Copy link
Member

@ijlee2 ijlee2 May 4, 2023

Choose a reason for hiding this comment

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

suggestion

After merging the pull requests that you've worked on, in order to update ember-qunit to be a v2 addon, I'd recommend that you update the configuration files such as .eslintignore, .eslintrc.js, .npmignore, etc., by matching the latest code from ember-cli blueprints.

https://github.com/ember-cli/ember-cli/tree/v4.12.1/blueprints/app/files

I think this will help us ensure that we don't develop code based on old standards.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure thing. Would like to merge and do a follow up tho

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create an issue to track the comments in this review before we merge this please? that way we know for sure it won't get lost.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

test-app/.eslintrc.js Outdated Show resolved Hide resolved
test-app/index.js Outdated Show resolved Hide resolved
Copy link
Member

@ijlee2 ijlee2 left a comment

Choose a reason for hiding this comment

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

I didn't pass the commit titled Initial extraction. I'd suggest the following steps:

  • Get help with merging #1060 and #1063
  • Rebase #1062 and #1064 (simplify the commit history)
  • Provide more context (e.g. clarify which parts of the code are something that you added, versus which are something that exist already), to help people unfamiliar with the project review the pull requests

What do you think?

@NullVoxPopuli
Copy link
Collaborator Author

Does knowing that the test-app is the addon, copied, help?

Just missed a couple files in the adaptation to a real app is all

@NullVoxPopuli
Copy link
Collaborator Author

Pr updated, you'll want to keep going through the commits tho, because each commit is not a at the level of a whole PR would be. later commits address issues in earlier commits.

test-app/README.md Outdated Show resolved Hide resolved
test-app/README.md Outdated Show resolved Hide resolved
@MelSumner MelSumner self-requested a review June 12, 2023 19:24
@MelSumner MelSumner merged commit d4e152e into emberjs:master Jun 12, 2023
@NullVoxPopuli NullVoxPopuli deleted the switch-to-pnpm branch June 12, 2023 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants