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

Fix triple-slash reference in complied output, breaking TS consumers #704

Merged
merged 6 commits into from
Oct 1, 2023

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Aug 29, 2023

Related fixes:

Any import from @ember/component (or its subpaths), where @ember/component types are exposed in the declarations, ends up inserting /// <reference> entries at the top of the file.

image

This made upgrading to v1.0.2 of ember-async-data impossible for folks using any ember-source version that has a different set of types than the ember-source version that ember-async-data is developed with. (too new, no preview types, provided by stable. too old, no preview types, provided by @types/*).

Locally, to test, all you need to do is

yarn build
cat ember-async-data/declarations/helpers/load.d.ts

and observe that there are no triple-slash directives.

@SergeAstapov
Copy link
Collaborator

@NullVoxPopuli thank you for doing this here! Had the exact same thought.

@NullVoxPopuli one concern though about support:
readme currently says Ember.js v3.28 or above (requires Octane Edition) hence I assume we need to add ember-functions-as-helper-polyfill to avoid major release

@NullVoxPopuli
Copy link
Contributor Author

hence I assume we need to add ember-functions-as-helper-polyfill to avoid major release

just added! thanks!

@SergeAstapov SergeAstapov added the bug Something isn't working label Aug 29, 2023
Copy link
Collaborator

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

If you do as suggested here and explicitly name the type, I believe you can drop the other changes here, which would be preferable for now. Switching to the plain function as helper can come in a later PR that way, and avoid introducing any other changes for this one TS fix. Do let me know if that doesn't work, though!

Co-authored-by: Chris Krycho <hello@chriskrycho.com>
@SergeAstapov
Copy link
Collaborator

If you do as suggested here and explicitly name the type, I believe you can drop the other changes here, which would be preferable for now.

as per #705 it does not seem viable.

@chriskrycho updated this PR per the plan outlined in #705 (comment)

please let us know if this looks good to proceed or if you have any concerns. Thank you for suggestions and ideas here and in the other PR!

@SergeAstapov
Copy link
Collaborator

@chriskrycho per the plan outlined in #705 (comment), this is now ready to go if no concerns from your side and then we can publish v1.0.3

@SergeAstapov SergeAstapov merged commit 8771459 into tracked-tools:main Oct 1, 2023
13 checks passed
@NullVoxPopuli
Copy link
Contributor Author

Made a tool to help out in the future: https://github.com/NullVoxPopuli/fix-bad-declaration-output

@chriskrycho
Copy link
Collaborator

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants