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

Migrate to v2 addon format #50

Merged
merged 9 commits into from
Jul 17, 2023
Merged

Migrate to v2 addon format #50

merged 9 commits into from
Jul 17, 2023

Conversation

dfreeman
Copy link
Member

@dfreeman dfreeman commented Jul 6, 2023

This is the first of what will likely be a sequence of breaking-change PRs leading up to ember-exclaim@2.0. The goal is to be as un-disruptive as possible in what we actually break, while still taking opportunities to modernize the codebase and improve overall DX/performance.

Notable breaking changes in this PR:

  • dropping Ember < 3.28 from our test matrix
  • requiring Embroider or ember-auto-import@2 in the consuming package, as we're now a v2 addon

Note that the general structure for v2 addons is a monorepo with one workspace for the addon itself and another for a test application (what used to be the "dummy app"). The addon is "just" an npm package that uses Rollup for its build process, while the test app is a standard Ember application with none of the weirdness associated with the hybrid EmberAddon/dummy application setup from v1 addons.

This PR also introduces a third playground-app workspace, which contains the stuff we deploy to GitHub pages to demo Exclaim. One nice side effect of the v2 setup is we don't have to force our tests and docs/demos/etc to coexist in a single application.

I've done my best to break things out by commit, but unfortunately the first one in particular is large and a bit entangled.

  • 3fd75e0: migrate into the v2 addon structure generated by https://github.com/embroider-build/addon-blueprint
    • most of the updates here either boilerplate generated code or file moves, but a few things were manual changes I'll call out
    • internal import paths were updated to be relative to adhere to standard Node/bundler resolution rules
    • migrating from assert.equal to assert.strictEqual in tests—this shouldn't have had to be part of this commit, but ended up being difficult to extract after the fact
  • 49c06d4: turn off some lint rules that guard against the use of old Ember APIs. We definitely want to fix these, but not as part of this already-large PR.
  • 9f3292b: fix imports from @ember/string that moved to @ember/template a long long time ago
  • 94a58cd: clean up a few unneeded dependencies from the test-app workspace
  • c24dc87: split the demo routes out of test-app into a separate playground-app workspace so that we don't need to maintain dependencies (and juggle ember-try scenarios) for both things together
  • b9927a2: upgrade to ember-ace v3, which is much easier to use in modern apps since it cleans up use of some old Ember APIs and is itself a v2 addon that makes use of standard bundler features now instead of custom ember-cli-build.js configuration
  • 508d2ed: clean up our ember-try scenarios and get CI passing
  • abe5857: add our standard "Salsify FE team" automated release configuration

# always run CI for tags
tags:
- '*'
- main
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@@ -1,6 +1,6 @@
The MIT License (MIT)

Copyright (c) 2019
Copyright (c) 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

lol time flies

],
"scripts": {
"build": "rollup --config",
"lint": "concurrently 'npm:lint:*(!fix)' --names 'lint:'",
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh neat I was not familiar with concurrently

Copy link
Member Author

Choose a reason for hiding this comment

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

Tbh I find the intermingled output a little rocky sometimes, but it's better than having to spin up separate shells and (more importantly) it's what the blueprint does by default, so I went with it

@dfreeman dfreeman merged commit a12b18a into main Jul 17, 2023
7 checks passed
@dfreeman dfreeman deleted the v2-addon branch July 17, 2023 08:19
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.

None yet

2 participants