Skip to content

Conversation

@wayfarer3130
Copy link
Contributor

@wayfarer3130 wayfarer3130 commented Jun 13, 2025

Any call to retrieveSeriesMetadata was failing because of an undeclared variable.
Also, re-linted api.js and fixed an eslint rule that is no longer valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer a valid code, so I applied the allowed value.

@pieper
Copy link
Contributor

pieper commented Jun 13, 2025

Would it be possible for you to separate the style changes from the substance changes into different commits for easier review?

src/api.js Outdated
const request = getRequestOptions(options.request)
return this._httpGetApplicationJson(url, {}, false, withCredentials);

const request = getRequestOptions(options.request);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The series retrieve was passing false in instead of the request object that got created on the previous line.

@wayfarer3130
Copy link
Contributor Author

Would it be possible for you to separate the style changes from the substance changes into different commits for easier r

Would it be possible for you to separate the style changes from the substance changes into different commits for easier review?

Done.

@pieper
Copy link
Contributor

pieper commented Jun 13, 2025

Would it be possible for you to separate the style changes from the substance changes into different commits for easier review?

Done.

Thanks - 👍

Let me know when you are finished tweaking and I'll review.

@wayfarer3130
Copy link
Contributor Author

@pieper - I'm done tweaking if you could review.
I have a couple of other issues to fix, but I'm going to create another PR for those - it is a lot easier to manage several small changes.

Copy link
Contributor

@pieper pieper left a comment

Choose a reason for hiding this comment

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

I'm just curious about the lint config, otherwise it looks fine.

extends: ['airbnb-base', 'prettier'],
rules: {
'import/extensions': "always", // Better for native ES Module usage
'import/extensions': 2, // Better for native ES Module usage
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why the symbol is changed to a magic number? What does 2 mean?

@pieper
Copy link
Contributor

pieper commented Jun 16, 2025

Also it looks like the tests aren't running because the ubuntu version is old. Can we just change it to a newer version?

@wayfarer3130
Copy link
Contributor Author

@pieper - I'm not sure how to get it building, something in the import isn't quite right. Is it ok if I just use bun to build it instead? That build is quite fast, and is what i normally use to build it locally. Reduces the dependencies on things like rollup.

@pieper
Copy link
Contributor

pieper commented Jun 16, 2025

@wayfarer3130 yes, if you know a way to get it working that's fine with me. I'm not up to date on those tools.

@wayfarer3130 wayfarer3130 merged commit 214a9ab into master Jun 17, 2025
4 checks passed
@wayfarer3130 wayfarer3130 deleted the fix/withCredentials-missing branch June 17, 2025 12:20
@github-actions
Copy link

🎉 This PR is included in version 0.11.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants