-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Added new libs #81
Added new libs #81
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't looked at it in great detail, as for nimiq-utils
we don't necessarily require a review. But if it's one of your first times contributing to it, a PR is a good idea of course 👍
- Adding some tests would be good.
- I'd rename "fork" to "migration" everywhere.
- Should add some documentation in the Readme that your new modules are meant to be imported differently.
Mmmm okay. I think these libraries can also be used in the wallet and by other people in the community, so I'd like to hear the team's opinion as well. If anyone think there is a better naming, code structure, API...
I could, but I don't know how to calculate the real value. I asked core if they have unit testing in the Rust implementation, but they do not :(
Ok
I was thinking instead of adding a page in the dev centre with all the utilities of this npm package. |
package.json
Outdated
"exports": { | ||
"./rewards-calculator": { | ||
"types": "./dist/src/rewards-calculator/rewards-calculator.d.ts", | ||
"import": "./dist/module/rewards-calculator.js", | ||
"require": "./dist/nomodule/rewards-calculator.js" | ||
}, | ||
"./albatross-policy": { | ||
"types": "./dist/src/albatross-policy/index.d.ts", | ||
"import": "./dist/module/albatross-policy.js", | ||
"require": "./dist/nomodule/albatross-policy.js" | ||
}, | ||
"./supply-calculator": { | ||
"types": "./dist/src/supply-calculator/supply-calculator.d.ts", | ||
"import": "./dist/module/supply-calculator.js", | ||
"require": "./dist/nomodule/supply-calculator.js" | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe when you add these exports, the default export becomes impossible. So please test if with this you can still import all the other existing utils.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still have the index declare somewhere else, right?
https://github.com/nimiq/nimiq-utils/blob/onmax/pos-utils/package.json#L8-L10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I add pkg.new to this project in this PR?
Yes, of course, these utils are provided with the idea that they are useful across multiple projects and also to the community. I also didn't blindly hit the approve button 😄 I did look at it, just not in full detail, and did not check necessarily what each method does. You worked with these things, and if you think the API structure makes sense this way, I trust you there. Also the change in how to import things makes sense arguably, even if it leads to inconsistency between the older code and the new code. The nicest would obviously be to also update the old code, but if we chose to not update the old code in the interest of time, there should be at least some mention of the two different ways to use these utils in the readme, and that newer utils will only be provided in the new format. Those new utils can then also follow the new file naming convention that you are using. A good middle ground could be to not update the old code (e.g. to be individual functions instead of static methods etc.) but to simply provide an export rule for each sub folder in |
6ad0870
to
a38cae8
Compare
yes, I like that idea. I will do it |
src/main.ts
Outdated
import * as Cookie from './cookie/Cookie'; | ||
|
||
/** | ||
* @deprecated - Import from `@nimiq-utils/address-book` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is a typo in every comment in this file:
* @deprecated - Import from `@nimiq-utils/address-book` | |
* @deprecated - Import from `@nimiq/utils/address-book` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I have the changes in locally.
I didn't pushed yet, since I am having an issue with the @deprecated
comments getting stripped during build
time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still a valid comment. The package name is wrong in each comment.
97fe92b
to
cfaf9a3
Compare
I was investigating how we could use I could put a |
On the other hand, I took the opportunity to switch from So I decided to use the defaults from the output
I know this change is out of the scope of the PR, but it will helps us avoiding the barrel file while having a great DX. You might argue that this change breaks import via CDN, which you are right, but I don't see anyone doing that in GitHub so I think is quite safe to do. However, if you find this change unnecessary, I can always rollback. |
cfaf9a3
to
bd647a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also see a massive amount of new dependencies in yarn.lock, is that really necessary?
package.json
Outdated
"lint": "eslint src/**/*.ts tests/*.spec.ts", | ||
"lint:fix": "eslint src/**/*.ts tests/*.spec.ts --fix", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"lint:fix": "eslint src/**/*.ts tests/*.spec.ts --fix", | |
"lint:fix": "yarn lint --fix", |
src/main.ts
Outdated
import * as Cookie from './cookie/Cookie'; | ||
|
||
/** | ||
* @deprecated - Import from `@nimiq-utils/address-book` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still a valid comment. The package name is wrong in each comment.
Also please rebase on top of |
I found a way to deprecate the barrel file: 2757921 |
If you are fine with the change from rollup to unbuild, I can cleanup the commit history and let it ready to merge. Right now the PR is a bit messy |
Yes, I think unbuild is fine. Let's modernize this! 👍 |
b1e2249
to
af01087
Compare
Ready to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are we gaining from the switch to unbuild
? I mean, yes, our previous way of building was bad, where we transpiled via tsc
first and then bundled via rollup
, but this could have been done by rollup
with a proper typescript plugin as well.
In the unbuild
readme I'm seeing "Integration with mkdist for generating bundleless dists with file-to-file transpilation.". That sounds good for tree shaking and your individual export rules in package.json
, but we also kind of had that with our previous config. Is that even enabled now for unbuild
, as for example the RateLimitScheduler
(which is missing in the config file) did not receive its own chunk, also with unbuild
.
I'm fine with using unbuild
if you guys think it's good, but I'm not seeing the advantage over rollup
yet.
Regarding the other changes:
The introduction of individual export rules I like a lot as we discussed earlier. Some seem to be missing though and need to be added.
There were several issues introduced with the change of the main.ts
file and the idea itself is not optimal. I think this should be reverted.
build.config.ts
Outdated
entries: [ | ||
'src/main', | ||
'src/address-book/AddressBook', | ||
'src/browser-detection/BrowserDetection', | ||
'src/currency-info/CurrencyInfo', | ||
'src/clipboard/Clipboard', | ||
'src/cookie/Cookie', | ||
'src/fiat-api/FiatApi', | ||
'src/formattable-number/FormattableNumber', | ||
'src/request-link-encoding/RequestLinkEncoding', | ||
'src/tweenable/Tweenable', | ||
'src/utf8-tools/Utf8Tools', | ||
'src/validation-utils/ValidationUtils', | ||
'src/rewards-calculator/rewards-calculator', | ||
'src/albatross-policy/albatross-policy', | ||
'src/supply-calculator/supply-calculator', | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RateLimitScheduler is missing here. (But it was previously also missing in the rollup config). Others might be missing, too.
package.json
Outdated
"exports": { | ||
".": { | ||
"types": "./dist/main.d.ts", | ||
"import": "./dist/main.mjs", | ||
"require": "./dist/main.cjs" | ||
}, | ||
"./address-book": { | ||
"types": "./dist/address-book/AddressBook.d.ts", | ||
"import": "./dist/address-book/AddressBook.mjs", | ||
"require": "./dist/address-book/AddressBook.cjs" | ||
}, | ||
"./browser-detection": { | ||
"types": "./dist/browser-detection/BrowserDetection.d.ts", | ||
"import": "./dist/browser-detection/BrowserDetection.mjs", | ||
"require": "./dist/browser-detection/BrowserDetection.cjs" | ||
}, | ||
"./clipboard": { | ||
"types": "./dist/clipboard/Clipboard.d.ts", | ||
"import": "./dist/clipboard/Clipboard.mjs", | ||
"require": "./dist/clipboard/Clipboard.cjs" | ||
}, | ||
"./cookie": { | ||
"types": "./dist/cookie/Cookie.d.ts", | ||
"import": "./dist/cookie/Cookie.mjs", | ||
"require": "./dist/cookie/Cookie.cjs" | ||
}, | ||
"./currency-info": { | ||
"types": "./dist/currency-info/CurrencyInfo.d.ts", | ||
"import": "./dist/currency-info/CurrencyInfo.mjs", | ||
"require": "./dist/currency-info/CurrencyInfo.cjs" | ||
}, | ||
"./fiat-api": { | ||
"types": "./dist/fiat-api/FiatApi.d.ts", | ||
"import": "./dist/fiat-api/FiatApi.mjs", | ||
"require": "./dist/fiat-api/FiatApi.cjs" | ||
}, | ||
"./formattable-number": { | ||
"types": "./dist/formattable-number/FormattableNumber.d.ts", | ||
"import": "./dist/formattable-number/FormattableNumber.mjs", | ||
"require": "./dist/formattable-number/FormattableNumber.cjs" | ||
}, | ||
"./request-link-encoding": { | ||
"types": "./dist/request-link-encoding/RequestLinkEncoding.d.ts", | ||
"import": "./dist/request-link-encoding/RequestLinkEncoding.mjs", | ||
"require": "./dist/request-link-encoding/RequestLinkEncoding.cjs" | ||
}, | ||
"./tweenable": { | ||
"types": "./dist/tweenable/Tweenable.d.ts", | ||
"import": "./dist/tweenable/Tweenable.mjs", | ||
"require": "./dist/tweenable/Tweenable.cjs" | ||
}, | ||
"./utf8-tools": { | ||
"types": "./dist/utf8-tools/Utf8Tools.d.ts", | ||
"import": "./dist/utf8-tools/Utf8Tools.mjs", | ||
"require": "./dist/utf8-tools/Utf8Tools.cjs" | ||
}, | ||
"./validation-utils": { | ||
"types": "./dist/validation-utils/ValidationUtils.d.ts", | ||
"import": "./dist/validation-utils/ValidationUtils.mjs", | ||
"require": "./dist/validation-utils/ValidationUtils.cjs" | ||
}, | ||
"./rewards-calculator": { | ||
"types": "./dist/rewards-calculator/rewards-calculator.d.ts", | ||
"import": "./dist/rewards-calculator/rewards-calculator.mjs", | ||
"require": "./dist/rewards-calculator/rewards-calculator.cjs" | ||
}, | ||
"./albatross-policy": { | ||
"types": "./dist/albatross-policy/albatross-policy.d.ts", | ||
"import": "./dist/albatross-policy/albatross-policy.mjs", | ||
"require": "./dist/albatross-policy/albatross-policy.cjs" | ||
}, | ||
"./supply-calculator": { | ||
"types": "./dist/supply-calculator/supply-calculator.d.ts", | ||
"import": "./dist/supply-calculator/supply-calculator.mjs", | ||
"require": "./dist/supply-calculator/supply-calculator.cjs" | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the RateLimitScheduler
is missing, too.
src/main.ts
Outdated
export * from './validation-utils/ValidationUtils'; | ||
/* eslint-disable max-len */ | ||
import { AddressBook as _AddressBook } from './address-book/AddressBook'; | ||
import * as _BrowserDetection from './browser-detection/BrowserDetection'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes the import to be namespaced. The default export will then be available under _BrowserDetection.default
, not _BrowserDetection
. I.e. this changes the behavior in an undesired fashion with regards to what it was previously.
src/main.ts
Outdated
import * as _Tweenable from './tweenable/Tweenable'; | ||
import * as _Utf8Tools from './utf8-tools/Utf8Tools'; | ||
import * as _ValidationUtils from './validation-utils/ValidationUtils'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also creates namespaces which are different to the previous exports.
src/main.ts
Outdated
setCoinGeckoApiUrl as _setCoinGeckoApiUrl, | ||
} from './fiat-api/FiatApi'; | ||
import { FormattableNumber as _FormattableNumber, toNonScientificNumberString as _toNonScientificNumberString } from './formattable-number/FormattableNumber'; | ||
import { Priority as _Priority, RateLimitPeriod as _RateLimitPeriod, RateLimitScheduler as _RateLimitScheduler, RateLimitPeriodUsages as _RateLimitPeriodUsages, RateLimits as _RateLimits } from './rate-limit-scheduler/RateLimitScheduler'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is longer than 120 chars.
src/main.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity: Can AI generate such code or is it a lot of manual, tedious work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/main.ts
Outdated
/** | ||
* @deprecated Change it to: `import { Priority } from '@nimiq/utils/rate-limit-scheduler';` | ||
*/ | ||
export const Priority = _Priority; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For names which represent multiple things, for example a value and/or a type and/or a namespace, so basically enums, classes, namespaces and things for which multiple declarations were manually merged (for example BrowserDetection
), this approach will not work, as you're exporting only the value here. As such, users will not be able to use these things as types or namespaces. You'll need to rely on manual declaration merging to reconsturct these things on the new name.
For example, for this enum I think you'll need to do something along these lines (will have to experiment a bit; also not sure whether each declaration will have to have the documentation comment, or maybe only the export):
/** | |
* @deprecated Change it to: `import { Priority } from '@nimiq/utils/rate-limit-scheduler';` | |
*/ | |
export const Priority = _Priority; | |
/** | |
* @deprecated Change it to: `import { Priority } from '@nimiq/utils/rate-limit-scheduler';` | |
*/ | |
const Priority = _Priority; | |
type Priority = _Priority; | |
namespace Priority { | |
export type LOW = _Priority.LOW; | |
export type HIGH = _Priority.HIGH; | |
} | |
export Priority; |
As mentioned, you'd need to do that on all enums, classes, namespaces and anything manually merged. Quite some effort, just to add the @deprecated
annotation. Also, this entire file is super hard to maintain, as for each added or modified export, one has to remember to also replicate the change here. Even for changd enum values, like new fiat currencies being added to the FiatApi
.
I would say export it how it was done previously and get rid of the @deprecated
annotations, or try to tell the bundler / minifier to preserve these comments.
(I'm skipping review of the remainder of this file, as I think overall this is not the best approach due to the mentioned issues.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's really only for the deprecated attribute, then I agree with Daniel, that it's not worth it to break all existing usages, nor to add countless lines of boilerplate.
Thank you Daniel for the review and the detailed explanations. I really appreciate it, although I don't fully understand it, so I will continue to read about these topics. I will revert the changes in main.ts. Not worth it, as you both mention.
I am a monkey, so I like the fact that I have a tool that tells me that I might be doing something wrong. For example, I am adding `rate limiter' as you mention. I did it wrong and got this warning:
So, it is easy to me spot an issue and solve it. also it is easier to setup and we don't need third parties libraries like |
d4d09d6
to
b223d6e
Compare
Reverted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready to be merged or be rebased onto master, up to your liking.
You'll want to bump the package version as the version you set currently is already taken on master
. That can be done just before releasing the new package in a separate commit which only bumps the version and has the new version as commit message. That's what we typically do.
You mentioned previously, that instead of @deprecated
annotations, a message could be logged in main.ts
if the file is important. I think it's fine if you want to do that.
package.json
Outdated
@@ -1,24 +1,107 @@ | |||
{ | |||
"name": "@nimiq/utils", | |||
"version": "0.12.0", | |||
"version": "0.12.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: This version number is now already taken on the master
branch.
I can bumpp the version in the package.json, but not sure if I can publish on npm |
Please revert the version change, we'll bumb the version after this is merged. |
b223d6e
to
150f65d
Compare
done |
The code basically is a refactor + cleanup from the Staking Calculator with JS Docs on top of that.
I also update the bundling config a bit.
For the new libs, instead of using barrels files, we just use a submodule like:
very similar approach to the #37 that never got merged 🥲