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

Docs: add info about merged app directory #780

Closed
chriskrycho opened this issue Jul 31, 2019 · 19 comments
Closed

Docs: add info about merged app directory #780

chriskrycho opened this issue Jul 31, 2019 · 19 comments

Comments

@chriskrycho
Copy link
Member

We currently document one gotcha for addon authors building addons in TypeScript:

Because addons have no control over how files in app/ are transpiled, you cannot have .ts files in your addon's app/ folder.

To expand on that for future readers who may stumble on this (and as a starting point for the docs we should write up):

Having .js supplied to a specific path inside the app namespace by both an addon and the app itself, when one of them is using TypeScript to generate that file, creates a caching problem. When addons inject .js files into the app namespace, and we're compiling .ts files to .js files in the consuming app (always in the app namespace), or vice versa, we can end up in a situation where Ember CLI sees that there is already a .js file there, so it ignores the one generated from .ts. Which one it picks up seems to be semi-random (at least: we haven't chased it down to see if it's consistently replicable).

The real solution here is for addons to stop injecting themselves into the app namespace and to supply imports people can use instead to explicitly opt into using their functionality. However, until that becomes a community norm (best guess: not until Embroider lands), we should document this as a gotcha, including the kinds of build errors it produces, so that people find it when it happens to them and we have a useful place to direct addon authors to in understanding why and how they're breaking their consumers.

Ecosystem issues

@lifeart
Copy link

lifeart commented Jul 31, 2019

// cc

I think it should be redlined and pinned in the docs - because this is floating behaviour and can create serious damage for business.

for case - we have application.ts route and have ember-simple-auth installed - it's so easy to get issue with inconsistent builds, because there is no user action needed for it. (and alot of ember users use ember-simple-auth)

@jordpo
Copy link

jordpo commented Jan 6, 2020

I've also gotten bitten by the ESA Application route class export stomping my TS class.

@LuisAverhoff
Copy link

LuisAverhoff commented Jan 6, 2020

I've also experienced the same issue with ember-simple-auth stomping my TS application route class.

@patrickberkeley
Copy link

This is a big trap. Any app that consumes an addon that exports a file that matches the name of a file in the app will sporadically get a bad build.

ESA is one of the most popular add-ons, and it happens to export the application route. So that is why people have come across this issue using it.

Another very common case will be apps that consume app-specific addons (ones written just for one or two apps, not public consumption). The reason is this type of addon is often used to define a base which the one or more apps can extend. It is natural in this case for the files to have the same name in app and addon, and for there to be lots of them.

We were bitten by both the above scenarios (three apps use esa and consume two local add-ons). It was difficult to track down and cost us a bunch of time.

@lolmaus
Copy link

lolmaus commented Jan 20, 2020

Ran into this again. This time it's the Intl service from ember-intl: we customize it, but the original version gets loaded.

This happens only to me and does not happen to my teammates and CI. Extremely frustrating!

I strongly believe that, if ember-cli-typescript can't do anything about it, this must be recognized as an issue upstream and fixed there. Has the issue already been registered in the ember-cli repo or somewhere like that?

@lolmaus
Copy link

lolmaus commented Jan 20, 2020

Note that I can't use a different service name like app/services/intl-custom.ts. This would resolve the collision, but ember-intl tools, such as the t template helper, will still be using the original service!

Are you expecting me to fork ember-intl or what? I can't see a way out of this.

@lolmaus
Copy link

lolmaus commented Jan 20, 2020

OK, this seems to be the one: broccolijs/broccoli-persistent-filter#166

@patrickberkeley
Copy link

@lolmaus we have hacked around this by creating a -private directory for our TS files, then re-exporting from the a JS file at the normal position in the tree. Example for your situation:

// app/services/-private/intl.ts
// whatever code you need to customize…

Then re-export it in a JS file:

// app/services/intl.js
export { default } from `<your-app-or-addon-name>/services/-private/intl`;

This way you don't have two TS files with the same name in your tree.

@chriskrycho
Copy link
Member Author

Happily we have a way to at least give you a warning here (see the PR @dfreeman just opened earlier today). The issue here is unfortunately entirely upstream from us and there's nothing we can do but warn.

The medium-term solution here is for app re-exports for addons to stop being a thing, and to use normal imports and resolution instead.

@lolmaus
Copy link

lolmaus commented Jan 21, 2020

Maybe it's possible to manually remove JS files from the build via an in-repo addon?

@chriskrycho
Copy link
Member Author

Doing so would be inherently fragile and likely to break things, because people (for good or for ill) depend on that behavior across the ecosystem. You might be able to build something that works for your specific app, but unfortunately it wouldn't be generalizable. If you do, it'd be a great thing to blog about and link to from here for the sake of others hitting this pain point!

@lolmaus
Copy link

lolmaus commented Jan 21, 2020

Well, I'm basically asking if using broccoli-funnel in treeForApp is too late to remove the offending file from the build.

@ahemed-haneen
Copy link

This is a big trap. Any app that consumes an addon that exports a file that matches the name of a file in the app will sporadically get a bad build.

ESA is one of the most popular add-ons, and it happens to export the application route. So that is why people have come across this issue using it.

Another very common case will be apps that consume app-specific addons (ones written just for one or two apps, not public consumption). The reason is this type of addon is often used to define a base which the one or more apps can extend. It is natural in this case for the files to have the same name in app and addon, and for there to be lots of them.

We were bitten by both the above scenarios (three apps use esa and consume two local add-ons). It was difficult to track down and cost us a bunch of time.

so how can we solve this issue issue for ESA?

@chriskrycho
Copy link
Member Author

There are multiple suggested workarounds in this thread. Those are the best options available at present, unfortunately.

@patrickberkeley
Copy link

@theloosecannon this is our current approach: #780 (comment)

@JonRossway
Copy link

We ran into a similar issue with ember-redux. Our reducers/index.ts file needed to be renamed to index.js and the warning went away.

@chancancode
Copy link

chancancode commented Sep 2, 2020

We ran into this problem with app/services/store.{js,ts}. I think Ember Data provides a store.js in the app tree, if you care to override it it won't be used, otherwise the default implementation does what you want (re-export the ember data store as a service).

We did have a custom store subclass and hit this issue once we renamed it to .ts. The solution we went with (similar to what others have done) is to put our custom store in app/services/-store.ts, then add:

// app/services/store.js

export { default } from './-store';
export * from './-store';
// app/services/store.d.ts

export { default } from './-store';
export * from './-store';

I haven't tested that the .d.ts is actually necessary, or if TS will just follow the re-export from the js file and find the types. Either way, it seems to work well for us. It's unfortunate that it is necessary, but doesn't seem to have much other unwanted consequences.

@machty
Copy link

machty commented Dec 19, 2022

Have there been any updates as to the best practices to handle this, or perhaps modern / upcoming Ember APIs that might sidestep these issues?

abeforgit added a commit to lblod/ember-rdfa-editor-lblod-plugins that referenced this issue Apr 7, 2023
@chriskrycho
Copy link
Member Author

The workarounds described here remain (and will remain) the best options as long as Ember’s app-level build is still a “classic” build system reliant on the resolution and override rules it has used for roughly a decade now. When it is replaced with a v2 app authoring story (presumably using Vite or similar), the issue will be fundamentally resolved. In the meantime, this is inactionable on our end. Equally importantly, we now recommend that people switch to using ember-cli-babel for apps (as documented here) and the rollup plugin configured as part of the v2 add-on build for add-ons, in conjunction with running glint or tsc directly on their projects, so we are not further investing in fixing things like this in ember-cli-typescript. Closing accordingly!

@chriskrycho chriskrycho closed this as not planned Won't fix, can't repro, duplicate, stale Sep 28, 2023
Octane docs push! automation moved this from To do to Done Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

No branches or pull requests

10 participants