-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: esm rewrite #188
feat: esm rewrite #188
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.
Hey there @ericdeansanchez! This is a really good changeset, and there's not many outright problems here, honestly a lot of my comments were style things, or just improvements. I have some concrete points to discuss here:
Required to discuss before merging:
- Why 3.0? It seems a little strange to me that we are bumping a major version, if supposedly this is not meant to have any public API changes should have no impact to the end user?
- Files in dist.. We should be careful about changing the names of the generated files, and we should try keep backwards compatibility here since people most definitely will sometimes be importing directly from
dist
. - Minified output. We need to add a minified output to
dist
. - Different branch. I suggest we merge this PR into a
beta
branch, rather thanmain
, so that we're not blocked from releasing fixes onmain
after merging this, since this isn't ready for release yet. - Delete
src/imgix-core-js.js
since it's no longer used. - Add back Typescript types (and to
package.json
).
Style choices/feedback/improvements if you want:
- A lot of assertions are inconsistent in their ordering of the expected and actual results. The official order from the Node doc is
assert.equal(actual, expected)
. - Use
const
as the default, and only uselet
when you have to. In fact, for most the code I write these days, I rarely ever uselet
, and often I treat it as a code smell if I have to use it. For this reason I use IIFEs a lot, which I then refactor into their own functions or methods. - The tests in general can be cleaned up a lot. This is a bit out of scope to this PR so you can choose to ignore this, since you already have a lot going on here.
- There were a lot of lint/indentation changes in this PR. It would've been easier to review if these were kept to a separate PR, so the signal/noise ratio was higher.
Overall I think this is a really good first pass! Good job 👏
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.
Looks good! I agree with @frederickfogerty on all points. I'll throw in anything else I noticed/questions I have:
- I think we should move towards parameterizing the tests. I know it's out of scope here, but just wanted to add that 2¢ somewhere before I forget.
- In addition to the TS definitions, is there any reason not to have the main file as
.ts
at this point? At least that way, we can benefit from using TS when developing. - This is super nitpicky but I can't help but notice that some files have a newline at the end of the file while others don't. We should make that consistent, and configure prettier to enforce it in the future.
f2a42b5
to
c488e43
Compare
Features
Bug Fixes
Code Refactoring
Build System
Continuous Integration
ContributorsCommit-Lint commandsYou can trigger Commit-Lint actions by commenting on this PR:
|
Co-authored-by: sherwinski <[email protected]>
Congats @ericdeansanchez 👏 this was a big effort and major contribution to this library |
🎉 This PR is included in version 0.3.0-beta.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
The purpose of this PR is to begin the transition to ES modules.