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

Esm Migration + bump libraries #646

Closed
wants to merge 38 commits into from
Closed

Esm Migration + bump libraries #646

wants to merge 38 commits into from

Conversation

jbigman
Copy link
Contributor

@jbigman jbigman commented Aug 3, 2023

Dependencies

Bump got from 11.8.6 to 13.0.0
Bump mocha from 9.1.4 to 10.2.0
And minor updates.

ESM migration

Not that much to say.

jbigman and others added 30 commits July 26, 2023 13:11
- fixed search: more section title is not always empty, it can contain "Apps"
- fixed missing comments (ds:9 => ds:8)
- new url called to test throttle,
- updated expected results
# Conflicts:
#	package-lock.json
/**
         * Schedules execution of a one-time `callback` after `delay` milliseconds. The `callback` will likely not be invoked in precisely `delay` milliseconds.
         * Node.js makes no guarantees about the exact timing of when callbacks will fire, nor of their ordering. The callback will be called as close as possible to the time specified.
         * When `delay` is larger than `2147483647` or less than `1`, the `delay` will be set to `1`. Non-integer delays are truncated to an integer.
         * If `callback` is not a function, a [TypeError](https://nodejs.org/api/errors.html#class-typeerror) will be thrown.
         * @SInCE v0.0.1
         */
@facundoolano
Copy link
Owner

facundoolano commented Aug 3, 2023

Thanks for this contribution, but this PR is kind of big and it's fixing several separate things at once. Can you make separate PRs so it's easier to review and verify nothing breaks along the way?

For example, the title says it fixes the tests but I see the build is still failing. The ideal would be to get a PR to fix the tests and once that is working get a separate one for the esm so we can see that nothing breaks, and separate ones for library bumps, etc.

@jbigman
Copy link
Contributor Author

jbigman commented Aug 3, 2023

I'm sorry, the last commit failed because of wrong assertion. But everything is fine.

It's my firt time doing pull requests, i don't know how to do multiple requests in parallel. I'm looking on how to do that

@facundoolano
Copy link
Owner

Well, my suggestion is to work at one of the problems at a time. You can start by the simplest one if you prefer, to make it easier to get things merged before getting into the complicated parts. For example, the mapping updates should be simple to review on their own, you can branch from the current main and apply only those changes, then wait for that to get merged before moving to the next PR.

@jbigman
Copy link
Contributor Author

jbigman commented Aug 3, 2023

This current PR adds Esm migration and bumps. Do you want to fix imports before bumps ?

@jbigman jbigman changed the title Esm Migration + fix throttle tests + added properties in mapping + bump libraries Esm Migration + bump libraries Aug 4, 2023
@facundoolano
Copy link
Owner

Please pull from main to get a clean diff. Also, yes it would be better to separate the version bumps so this is as small as possible for review

const scriptData = require('./utils/scriptData');
const { BASE_URL } = require('./constants');
const helper = require('./utils/mappingHelpers');
import { assoc, path } from 'ramda';
Copy link
Owner

Choose a reason for hiding this comment

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

I noticed that you replaced a lit of module imports eg R for direct imports of module functions. Please don't do that, as we lose the information of where each function is coming from, making the code less readable

@facundoolano
Copy link
Owner

Closing this one as ESM migration is happening in #651

@jbigman jbigman deleted the main branch August 7, 2023 21:02
@jbigman jbigman restored the main branch August 7, 2023 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants