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

[WIP] - Convert to v2 Addon #1117

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cloke
Copy link

@cloke cloke commented Sep 6, 2024

Converts to a v2 add-on. The test app builds and works, but still working through some of the ember-try scenarios.

Also switches to pnpm from yarn. This solved some babel errors that popped up and brings it more in line with the general ember add-on tooling.


This pull request includes significant changes to the CI configuration, dependency management, and documentation. The most important changes include switching from Yarn to pnpm for dependency management, updating the CI workflow to cache dependencies and use pnpm, and removing extensive documentation and configuration files.

CI Configuration and Dependency Management:

  • .github/workflows/ci.yml: Updated CI workflow to use pnpm instead of Yarn for installing dependencies, running lint, and executing tests. Added a new job to cache dependencies using pnpm. [1] [2] [3]
  • CONTRIBUTING.md: Updated commands to use pnpm instead of Yarn for installing dependencies, linting, and running tests.

Documentation and Configuration Cleanup:

  • .npmignore: Removed extensive list of files and directories to be ignored.
  • docs/: Removed detailed documentation for did-intersect and scroll-into-view modifiers. [1] [2]
  • ember-cli-build.js: Removed the build configuration file.

Miscellaneous:

@elwayman02
Copy link
Owner

Thanks for getting started on this! I'm on board with switching to pnpm, but I think we're going to need to move rather than remove the documentation before we push this across the finish line. The docs/ directory powers our documentation site published through Netlify, and we don't want to lose the docs that have been written. However, in the context of a V2 addon, I think the ideal setup is do have ember-scroll-modifiers/, test-app/, and docs-app/ (or demo-app/). This will improve a lot of things, including our embroider build scenarios, because right now a lot of our embroider issues come from having field-guide as a devDependency to power the docs. Moving that to a separate docs-app/ package will mean our tests don't have that dependency and embroider builds will correctly pass (or at least reveal real issues in the published package).

@cloke
Copy link
Author

cloke commented Sep 6, 2024

Just to clarify are the docs what is in the test-app folder? I had to move that folder into test-app for things to work, but I could add demo-app to the workspace and go down that path.

@elwayman02
Copy link
Owner

The docs are what you deleted:

docs/: Removed detailed documentation for did-intersect and scroll-into-view modifiers. [1] [2]

_WIP__-_Convert_to_v2_Addon_by_cloke_·_Pull_Request__1117_·_elwayman02_ember-scroll-modifiers

I don't see them moved to the test-app/, they appear to just be removed entirely by this PR.

@cloke
Copy link
Author

cloke commented Sep 6, 2024

I totally forgot to do a git add after they got moved... sorry for the confusion.

@elwayman02
Copy link
Owner

elwayman02 commented Sep 6, 2024

Right as I say that, I see you realized the same thing and pushed it in a new commit. 👍 No worries! Yes, docs/ needs to be at the root of whatever app they're part of, which can be test-app/ but if moved to a separate docs-app/ would fix some (or possibly all) of the embroider test scenario failures. That said, as long as they exist somewhere that I can deploy, I'd consider it a non-blocker for the purpose of V2 conversion. If we leave them in the test-app/ for now, we can file an issue to move them to a separate app in the workspace later, so whatever works is fine. I was more concerned about the possible loss of the docs than anything else.

- uses: actions/checkout@v4
- uses: pnpm/action-setup@v4
- name: Install Node.js
uses: actions/setup-node@v4
Copy link
Owner

Choose a reason for hiding this comment

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

Prefer to use volta as in the other existing builds in this file:

      - name: Install Node
        uses: volta-cli/action@v4

This ensures the CI build uses the same version of node that developers would have running locally (if they have volta installed)

@cloke cloke force-pushed the convert-to-v2 branch 3 times, most recently from ab3f97b to de8a4b7 Compare September 9, 2024 15:22
@elwayman02
Copy link
Owner

I'm ok with dropping the ember-try scenarios for 3.28, 4.0, and 4.4 if they have unique failures. Focus on fixing 5.4 and 4.12 scenarios first, and then 4.8. If any of the other scenarios are still failing after that, you can delete them. We're going to have a breaking change for the V2 conversion anyway, and those Ember versions are EOL at this point.

"crypto"
],
"engines": {
"node": ">= 18",
Copy link
Owner

Choose a reason for hiding this comment

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

I believe node engine no longer needs to be specified in V2 addons?

"engines": {
"node": "16.* || >= 18"
},
"volta": {
Copy link
Owner

Choose a reason for hiding this comment

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

This is a piece you'll want to keep (but obviously replacing yarn with pnpm).

See also: https://github.com/elwayman02/ember-sinon-qunit/blob/master/package.json#L58

},
"homepage": "https://ember-scroll-modifiers.jhawk.co/",
"release-it": {
Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

We'll need to add the workspace support for it as well: https://github.com/elwayman02/ember-sinon-qunit/blob/master/package.json#L34

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.

2 participants