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 #256 #297

Merged
merged 7 commits into from
Feb 29, 2024
Merged

fix #256 #297

merged 7 commits into from
Feb 29, 2024

Conversation

hrueger
Copy link
Contributor

@hrueger hrueger commented Aug 27, 2023

by importing .js file and therefore removing the trailing slash

I'm also using Svelte Kit and currently patching this via yarn patch.

and therefore removing the trailing slash
@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @hrueger to sign the Salesforce Inc. Contributor License Agreement.

@hrueger
Copy link
Contributor Author

hrueger commented Aug 27, 2023

I signed the CLA, but then I got an error. Can you please recheck?

@hrueger
Copy link
Contributor Author

hrueger commented Aug 28, 2023

Hm, did I break the tests? That's strange...

@awaterma
Copy link
Member

@hrueger -- can you try signing the CLA again? :) Hope you are doing well!

@hrueger
Copy link
Contributor Author

hrueger commented Jan 16, 2024

Yes, thanks. When clicking on the link and logging in with GitHub, I get this:
grafik

What to do?

Copy link
Member

@awaterma awaterma left a comment

Choose a reason for hiding this comment

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

This change looks good to me; but there's a file conflict. Can you please update your pull request @hrueger? Thank you!

@hrueger
Copy link
Contributor Author

hrueger commented Jan 18, 2024

@awaterma Now that the project is written in typescript, I get ts errors when importing from that javascript file since the @types/punycode packages seems to only support the index export... any idea?

Could not find a declaration file for module 'punycode/punycode.js'. 'a:/_Source/tough-cookie/node_modules/punycode/punycode.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/punycode` if it exists or add a new declaration (.d.ts) file containing `declare module 'punycode/punycode.js';`ts(7016)

@colincasey
Copy link
Contributor

@hrueger it seems like the userland punycode module doesn't play well with Typescript (mathiasbynens/punycode.js#132).

My suggested workaround would be to:

  • remove @types/punycode from package.json

  • add a custom declaration file like lib/punycode.d.ts which should look something like this:

    declare module 'punycode/punycode.js' {
      // copy the types declared here:
      // https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/punycode/index.d.ts
    }
  • change the punycode/ import to punycode/punycode.js

And, since we only use a very small part of the API from punycode, you could possibly just include the type declaration for toASCII and change the import to import { toASCII } from 'punycode/punycode.js'.

@hrueger
Copy link
Contributor Author

hrueger commented Jan 21, 2024

@colincasey I submitted a PR to DefinitelyTyped: DefinitelyTyped/DefinitelyTyped#68273
Let's see if this makes it through ;-)

wjhsf pushed a commit that referenced this pull request Feb 8, 2024
@hrueger
Copy link
Contributor Author

hrueger commented Feb 12, 2024

@colincasey My DefinitelyTyped PR was merged earlier today, so I updated the package and now the types should be fine 🎉

@colincasey colincasey merged commit 997fda0 into salesforce:master Feb 29, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants