-
Notifications
You must be signed in to change notification settings - Fork 42
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
Stop reexporting type files #2030
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
@@ -27,7 +27,7 @@ const plugins = [ | |||
// "app" tree. Things in here should also be in publicEntrypoints above, but | |||
// not everything in publicEntrypoints necessarily needs to go here. | |||
addon.appReexports([ | |||
'components/**/*.js', | |||
'components/**/!(*types).js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we are following this pattern for type location going forward, should I also update the helper and modifiers pattern below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the hds modifiers and helpers live under the same folder(s). which folder structure are you envisioning?
Types next to the files:
packages/components/src/helpers/hds-link-to-models.ts
packages/components/src/helpers/hds-link-to-models.types.ts
packages/components/src/modifiers/hds-clipboard.ts
packages/components/src/modifiers/hds-clipboard.types.ts
Creation of parent folders (is it possible? does it work in Ember?):
packages/components/src/helpers/hds-link-to-models/index.ts
packages/components/src/helpers/hds-link-to-models/types.ts
packages/components/src/modifiers/hds-clipboard/index.ts
packages/components/src/modifiers/hds-clipboard/types.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a very good question. I'm not sure what the answer is atm, and so maybe it's best not to address this now.
I'll try out the above options just to see what it looks like/what works. It would be nice if it could be consistent across all components/modifiers/helpers/etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with app reexports - if the types are removed does this mean that a consumer in a v1 addon can't import things from type files (like enums etc), or is that still possible since they are included in the public entrypoints above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya I believe that is handled by the publicEntrypoints
. CUT currently re-exports the types (whoops!) but we also have an index file that imports and exports all the types. That file is not part of the appReexports
but it is accessible from cloud-ui.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
appReexports
is a compatability layer tat only needed for v1 apps with hbs/js, with current setup it would still export types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently don't have type files for helpers and modifiers, so I wouldn't worry about it just now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a suggestion for the changelog entry, otherwise it's good to get in
@@ -27,7 +27,7 @@ const plugins = [ | |||
// "app" tree. Things in here should also be in publicEntrypoints above, but | |||
// not everything in publicEntrypoints necessarily needs to go here. | |||
addon.appReexports([ | |||
'components/**/*.js', | |||
'components/**/!(*types).js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently don't have type files for helpers and modifiers, so I wouldn't worry about it just now.
Co-authored-by: Alex <alex-ju@users.noreply.github.com>
@alex-ju thanks for the suggestion and review |
Feel free to merge whenever time allows |
π Summary
Updates the rollup glob pattern to exlude
types.js
files within thecomponents
folder.π οΈ Detailed description
It seems like rollup adds all the ts/js files within the components/ folder into the ember-addon/app-js bit in the package.json and expects there to a
default
export. I think this is because it expects all those files to be components as is the ember convention? This results in noisey errors for our consumers.I thought we could just add the types pattern under the
{ exclude: string[] }
options in theappReexports
but it did not work as I would have expected, or perhaps my glob was not right. So I ended up updating thepatterns
globs to exclude types.πΈ Screenshots
Before:
![Screenshot 2024-03-27 at 9 32 36β―AM](https://private-user-images.githubusercontent.com/5448834/317355895-f66e72ed-a683-4a3a-9274-b1c8560abb2e.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjEzNzk1MzUsIm5iZiI6MTcyMTM3OTIzNSwicGF0aCI6Ii81NDQ4ODM0LzMxNzM1NTg5NS1mNjZlNzJlZC1hNjgzLTRhM2EtOTI3NC1iMWM4NTYwYWJiMmUucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDcxOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA3MTlUMDg1MzU1WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9MjZiYTU0YmM1ODhmNmQzMzhiOTc2NWIxZWM0YzNjMGE3MTU5MGJiMWFmYjYwM2U2YjI5NTBhNWViZjQyNjQzOSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.--iL7aoMXRJGhh_ozPxVFUqRtnzeUxpt6xk9GAYjuuw)
After:
![Screenshot 2024-03-27 at 9 59 55β―AM](https://private-user-images.githubusercontent.com/5448834/317355903-d4985f39-2fc1-4eb2-840b-229e0e26a640.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjEzNzk1MzUsIm5iZiI6MTcyMTM3OTIzNSwicGF0aCI6Ii81NDQ4ODM0LzMxNzM1NTkwMy1kNDk4NWYzOS0yZmMxLTRlYjItODQwYi0yMjllMGUyNmE2NDAucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDcxOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA3MTlUMDg1MzU1WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9MGVjZjA2M2FjZjg3NTgwZmJlNmI3OTY3MWJjY2E2Y2UyMTQxYzJlZDIwNzU3Njg3NjEzZjg0ZjRlNTJjNjNkNiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.8mDM__aGtYZFd-QIImO5cjFH55jq-WLA-1NouWawzmg)
π External links
appReexports
π Component checklist
π¬ Please consider using conventional comments when reviewing this PR.