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 package.json#exports for types #54

Merged
merged 2 commits into from
Dec 18, 2023
Merged

fix package.json#exports for types #54

merged 2 commits into from
Dec 18, 2023

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Dec 15, 2023

Goal:

  • get publint and arethetypeswrong happy without disabling checks

Motivation:

  • using the latest 1.2.x release, I was running in to type errors -- likely due to a misunderstanding I had with the vast of array of goofiness TS likes to have between authoring format and output format (they're compiling with tsc)

How I tested:

  • Get the lints happy in this repo after removing the disables
    • However, due to limitations in wasm-pack, we can't get around the FalseESM rule, because wasm-pack does not emit node code in the ESM format. The APIs used for the "standalone" build also do not exist in node, so we can't just use "standalone" for everything. The real fix here is probably in wasm-pack to make their output less weird / dated.
  • Run local tests
  • Check the index.html (npm start, since we don't have browser tests)
  • I built the package here and yarn link
  • go over to my content-tag PR on the glint repo in the template-imports workspace and yarn link content-tag
  • run yarn build in the glint root and check for errors. (At the time of creating this PR there are no errors)

Things I encountered:

  • with the currently published package

    File '/home/nvp/Development/OpenSource/emberjs/glint/node_modules/content-tag/index.d.cts' 
         is not a module. [2306]
    

    which like.. true, because we're using require it shouldn't be a module -- at least not an ESM one -- which is how I interpret this -- which may be wrong.
    In this PR I've just deleted the index.d.cts
    I think I had originally thought that the index.d.cts could help with the FalseESM rule, but it did not.

  • return type of parse is an array, rather than a single object -- this was a bug when the types were introduced

  • when trying to use standalone.js as the entrypoint for cjs, we correctly get an error from TS

    packages/environment-ember-template-imports/-private/environment/preprocess.ts:16:30 - error TS1479: 
      The current file is a CommonJS module whose imports will produce 'require' calls;
      however, the referenced file is an ECMAScript module and cannot be imported with 
      'require'. Consider writing a dynamic 'import("content-tag")' call instead.
      
      To convert this file to an ECMAScript module, change its file extension to '.mts', 
      or add the field `"type": "module"` to 
      '/home/nvp/Development/OpenSource/emberjs/glint/packages/environment-ember-template-imports/package.json'.
    
    16 import { Preprocessor } from 'content-tag';
                                    ~~~~~~~~~~~~~
    

@@ -10,15 +10,13 @@
"type": "module",
"exports": {
".": {
"browser": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

browser was moved under import to reduce the complexity and repetition of having types under 3 separate entries.

@NullVoxPopuli NullVoxPopuli self-assigned this Dec 17, 2023
@NullVoxPopuli NullVoxPopuli requested review from ef4 and mansona December 17, 2023 18:58
@ef4 ef4 merged commit b559377 into main Dec 18, 2023
1 check passed
@ef4 ef4 deleted the fix-types-and-exports branch December 18, 2023 17:14
@NullVoxPopuli NullVoxPopuli added the bug Something isn't working label Dec 18, 2023
@github-actions github-actions bot mentioned this pull request Dec 18, 2023
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.

2 participants