Skip to content

Embroider: Port to v2 format: fully convert to v2 native format #63

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

Merged
merged 21 commits into from
Jan 10, 2025

Conversation

bantic
Copy link
Contributor

@bantic bantic commented Jan 9, 2025

The last step in the embroider guide: https://github.com/embroider-build/embroider/blob/main/docs/porting-addons-to-v2.md
This PR converts the addon into a fully v2-native format.

Some additional changes:

  • Remove the release-it and related dependencies. Releasing v0.8.0 using release-it had some issues, and the main benefit (imo) is the automatic changelog, but I was pretty dissatisfied with the changelog output from that release (https://github.com/Addepar/ember-json-viewer/blob/master/CHANGELOG.md). In the short-term it seems better to npm publish directly and use github's "releases" to publish release notes (like we did for v0.8.1: https://github.com/Addepar/ember-json-viewer/releases/tag/v0.8.1). Longer-term we could bring it back or use embroider/release-plan instead
  • There were a few rough spots due to the fact we still use yarn classic here. pnpm seems to be preferred and would have made things easier (I think):
    • yarn classic package-hoisting seemed to cause some issues. In particular @glimmer/component@v2 causes issues when used w/ ember before 4.8 (I think, maybe it was 4.12) and even though the ember-try scenario specified downgrading to component@v1, there were mysterious CI issues for those older ember versions. These were only fixed by specifying glimmer/component@v1 for all of the package.json files. I suspect this was a module-hoisting issue that would go away w/ a newer package manager
    • I had to add an extra cd addon/ && yarn build step to the CI. I think pnpm has better affordances to sync+build sibling pkgs in a monorepo
  • Refactor templates + component properties to obsolete not and inc helpers. This way they don't have to be re-exported as global helpers into the consuming app's namespace (which seems like it could cause collisions for apps; both those seem like very common helpers that might be in use already)

@bantic bantic changed the title Embroider: Port to v2 format: separate test-app Embroider: Port to v2 format: fully convert to v2 native format Jan 9, 2025
@bantic
Copy link
Contributor Author

bantic commented Jan 9, 2025

The last dangling CI issues were solved by downgrading all monorepo packages to glimmer/component v1 (from v2). I think the issues is that component v2 doesn't work w/ ember before 4.8 (or 4.12?), and we are using yarn classic here which was probably doing some sort of module hosting that caused the ember 3.28 and 4.4 to want component-v1 but actually be using component-v2 and failing.

@bantic bantic force-pushed the bantic/embroider-native-v2-convert-to-native branch 2 times, most recently from 8c05e4a to ca1a1fc Compare January 10, 2025 10:20
@bantic bantic force-pushed the bantic/embroider-native-v2-convert-to-native branch from 30cb76b to da4b59b Compare January 10, 2025 15:00
@bantic bantic marked this pull request as ready for review January 10, 2025 15:04
@bantic bantic requested a review from a team January 10, 2025 15:07
Copy link
Member

@mixonic mixonic left a comment

Choose a reason for hiding this comment

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

Wow!

@eliasdawson-addepar
Copy link
Contributor

Should probably update the contributing docs since those commands aren't correct anymore. There's probably a standard template for v2 addons that we can use or tweak. Moving to a v2 addon changes the developer workflow quite a bit too. For example, you have to run yarn start in the addon directory to watch for changes, and you have to build the addon before you can run tests.

Also running locally, I get a ton of dependency warnings and eslint errors in the test-app.

@bantic
Copy link
Contributor Author

bantic commented Jan 10, 2025

Also running locally, I get a ton of dependency warnings and eslint errors in the test-app.

@eliasdawson-addepar Can you give some more details on this?

@bantic bantic force-pushed the bantic/embroider-native-v2-convert-to-native branch from 7ec5877 to 1e4bdf3 Compare January 10, 2025 16:03
@bantic
Copy link
Contributor Author

bantic commented Jan 10, 2025

I screenshared w/ @eliasdawson-addepar . It looks like there were some "workspace-aggregator" style yarn warnings -- I think those are typical for yarn (at least for yarn classic) and safe to ignore.

We also get a warning from rollup about a possible broken sourcemap:
image

I'll look more closely at this and address in a follow-up. The related rollup doc is here, but this doesn't give me much to go on: https://rollupjs.org/troubleshooting/#warning-sourcemap-is-likely-to-be-incorrect

And the eslint errors mentioned occur when there's a .node_modules.ember-try directory -- the errors were from eslint attempting to lint files in that dir.
That's surprising because we should be .gitignore-ing that directory, and eslint should also be ignoring it as a result. But I'll look into this as a follow-up, too.

@eliasdawson-addepar
Copy link
Contributor

Also running locally, I get a ton of dependency warnings and eslint errors in the test-app.

@eliasdawson-addepar Can you give some more details on this?

Paired separately. The dependency warnings we believe to be harmless for the time being and at least partially expected due to state of ember ecosystem. There is a sourcemap warning when building the addon which @bantic is going to investigate as a followup - it might be ok/just console noise, but my only concnern is that we don't break sourcemaps if they work currently. The lint errors in the test app were in node_modules.ember-try which suggests dirty local state, but also suggests there may be a misconfiguration somewhere as those directories should be ignored.

@bantic bantic added this pull request to the merge queue Jan 10, 2025
Merged via the queue into master with commit 0aef4fb Jan 10, 2025
8 checks passed
@bantic bantic deleted the bantic/embroider-native-v2-convert-to-native branch January 10, 2025 16:18
@bantic
Copy link
Contributor Author

bantic commented Jan 10, 2025

I looked into the issue regarding the sourcemaps.

The sourcemaps do seem to be there and be working correctly. I loaded the docs-app and poked around:

image

The rollup warning mentions "rollup-hbs-plugin", which is defined here:
https://github.com/embroider-build/embroider/blob/main/packages/addon-dev/src/rollup-hbs-plugin.ts

I'm not familiar w/ embroider or rollup to know exactly what that is doing. I don't see anything explicit about a sourcemap there.
But my hunch is that the rollup warning is about the hbs templates not having their own sourcemap, and I think that's fine because they are not actually emitted directly into the dist build anyway. They are combined into the component JS files (which do have sourcemaps).
So I think this is fine but I will keep an eye on it.

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