Skip to content

Comments

Convert to V2 addon#303

Closed
bwbuchanan wants to merge 1 commit intoember-cli:masterfrom
MenloCoaching:v2-addon
Closed

Convert to V2 addon#303
bwbuchanan wants to merge 1 commit intoember-cli:masterfrom
MenloCoaching:v2-addon

Conversation

@bwbuchanan
Copy link

This is an attempt at converting ember-load-initializers to the v2 addon format. Feedback welcomed.

@bwbuchanan bwbuchanan force-pushed the v2-addon branch 4 times, most recently from e8cdbc8 to 1e97ef1 Compare January 21, 2022 12:46
@@ -3,7 +3,7 @@ language: node_js
node_js:

Choose a reason for hiding this comment

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

@bwbuchanan maybe just delete this file and provide GitHub actions config instead?

@SergeAstapov
Copy link

@bwbuchanan would you like to incorporate recommendations from https://github.com/embroider-build/embroider/blob/main/PORTING-ADDONS-TO-V2.md?

@NullVoxPopuli
Copy link
Contributor

@SergeAstapov is this something we should take over? 😅

@chancancode
Copy link

I suppose it doesn’t hurt, but does it really matter that this is v1 or v2 when the core thing that this addon does is iterating over require.entries to look for the initializers? Shouldn’t this functionality be implemented in embroider natively and make this addon unnecessary/no-op under embroider?

@SergeAstapov
Copy link

SergeAstapov commented Oct 16, 2023

@SergeAstapov is this something we should take over? 😅

@NullVoxPopuli I'd love gets this over the finish line! although this would fall somewhere in the queue of addons I do have maintainer access.
I can seek help of co-workers to get this over the finish line.
We just need someone who can land PRs and do the release.

I suppose it doesn’t hurt, but does it really matter that this is v1 or v2 when the core thing that this addon does is iterating over require.entries to look for the initializers?

@chancancode as this point in time, the main problem with this addon is the fact that it depends on ember-cli-typescript@2, which is problematic per #291 (comment). So at least maintenance like bumping that to v5 is overdue, which kinda brings a question if we should do one step over and convert to v2 which is not that hard then.

Shouldn’t this functionality be implemented in embroider natively and make this addon unnecessary/no-op under embroider?

TBH it always was a mystery to me why this addon even exists (maybe was a part of migration story from ember-app-kit to Ember CLI?).
having this baked into Embroider IMO sounds like a logical step long term.

@chancancode
Copy link

Yeah, if it's close and easy, like I said, I don't think it hurts to keep things up-to-date here. But personally, I think if this addon is still needed with Embroider/Polaris, then there is something deeply wrong. (Same deal as the fingerprinting stuff, etc.) So, personally, I think the priority should be making sure the functionality is implemented natively in Embroider.

@ef4
Copy link
Contributor

ef4 commented Oct 17, 2023

I think it's fine to make this change, and separately after we do https://github.com/emberjs/rfcs/blob/strict-es-module-support/text/0938-strict-es-module-support.md, use that to eliminate the dependence on requirejs.entries.

@ef4
Copy link
Contributor

ef4 commented Sep 25, 2024

Closing in favor of #306.

In summary:

  • this library does want to continue to exist for anybody who depends on addons that contain initializers that expect to run. - It has been extended to accept the set of modules explicitly as an argument, eliminating the requirement that you're using AMD.
  • Instead of converting from v1 to v2 addon, it can just be a plain NPM library, because it doesn't need any ember-specific stuff.

@ef4 ef4 closed this Sep 25, 2024
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.

5 participants