-
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
refactor: build native ESM #98
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.
Explainer comments + linked issues.
if (args[0].runner === 'node') { | ||
return { | ||
env: { | ||
NODE_OPTIONS: '--loader=esmock' |
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.
Adding this to accommodate: https://github.com/iambumblehead/esmock#:~:text=esmock%20is%20used%20with%20node%27s%20%2D%2Dloader
build: { | ||
config: { | ||
format: 'esm', | ||
banner: { | ||
js: '' | ||
}, | ||
footer: { | ||
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.
This is needed because: ipfs/aegir#1096
insert_final_newline = true | ||
charset = utf-8 | ||
indent_style = space | ||
indent_size = 2 |
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.
just for sanity
README.md
Outdated
<script type="module"> | ||
import { lookup } from 'https://cdn.jsdelivr.net/npm/[email protected]/dist/index.min.js'; | ||
const client = window.IpfsHttpClient.create({ | ||
host: 'ipfs.io', | ||
port: 443, | ||
protocol: 'https' | ||
}); | ||
console.log(await lookup(client, '66.6.44.4')) | ||
</script> |
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.
Once this releases as v9.0.0
module, we can use this, validated locally.
@@ -1,5 +1,4 @@ | |||
import * as geoip from '../src/index.js' | |||
import { create } from 'ipfs-http-client' |
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.
no longer used.
@@ -17,7 +17,7 @@ | |||
"dist" | |||
], | |||
"type": "module", | |||
"main": "src/index.js", | |||
"main": "dist/index.min.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.
now packages can map to the esm directly.
"test": "npm run test:node && npm run test:browser", | ||
"test:node": "aegir test --target node", | ||
"test:browser": "aegir test --target browser", | ||
"test:browser": "aegir test --target browser --files test/**/*.browser.spec.{js,cjs,mjs}", |
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 needs to happen because of: ipfs/aegir#1097 Also, the test files had to be renamed because of this.
@@ -47,6 +47,7 @@ | |||
"chai": "^4.3.6", | |||
"chai-as-promised": "^7.1.1", | |||
"csv-parse": "^5.3.0", | |||
"esmock": "^2.0.6", |
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 needed to rewire dependencies to validate behavior.
return { obj, block } | ||
} catch (e) { | ||
if (numTry < MAX_LOOKUP_RETRIES) { | ||
return await getObjAndBlockWithRetries(ipfs, cid, numTry + 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.
if we fail, we retry till the const value is reached.
if (decodeCallCount === 1) { | ||
throw new Error('Decode Failed') |
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.
Fail the decoding on the first try.
e827bad
to
4423251
Compare
@@ -44,8 +44,8 @@ jobs: | |||
- uses: actions/checkout@v2 | |||
- uses: microsoft/playwright-github-action@v1 | |||
- run: npm install | |||
- run: npx aegir test -t browser --bail --cov | |||
- run: npx aegir test -t webworker --bail | |||
- run: npx aegir test -t browser --bail --cov --files test/**/*.browser.spec.{js,cjs,mjs} |
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.
All changes in this file needs to happen because of: ipfs/aegir#1097 Also, the test files had to be renamed because of this.
Also, simplified default example to use gateway API, and not RPC
0407e45
to
c953962
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.
Thank you @whizzzkid, this is great!
I simplified README a bit, to point people at using gateways, if possible.
I feel it's as good as we can make it before Lisbon, I'm merging + will make a release.
ps. try to follow conventions from https://semantic-release.gitbook.io/semantic-release/#commit-message-format – our other JS projects expect that, and it makes it easy to do release automation in the future (I've updated title of this PR)
Co-authored-by: Marcin Rataj <[email protected]> BREAKING CHANGE: minified version is ESM now
Follow Up To: #93
In this PR: