-
Notifications
You must be signed in to change notification settings - Fork 60
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: build esm modules using ipjs #865
Conversation
Probably |
b4b177f
to
0a3aa0a
Compare
test/build.js
Outdated
const tempy = require('tempy') | ||
|
||
describe('build', () => { | ||
if (process.platform !== 'win32') { |
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.
ipjs
is not supporting windows, trying to figure out why
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.
mikeal/ipjs#15 I can't simulate locally nor with a CI branch, so can we just run build on linux+mac while this gets figured out?
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 will stop us running tests on windows though, right? Or can we run tests under pure ESM instead?
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.
well, this just makes us not test that ipjs
is doing it is thing and building ESM to a dist with esm
and cjs
, as the only available test is that one.
It will currently not work on windows and that is a concern to have
We need to generate types from We can't generate types from the files in |
7db225c
to
a9d879d
Compare
ff580fe
to
2892995
Compare
d4e519e
to
4b23e2e
Compare
6e6d148
to
cf254fe
Compare
7698a49
to
8501679
Compare
8501679
to
2f08d66
Compare
test/dependency-check.js
Outdated
@@ -60,7 +60,7 @@ describe('dependency check', () => { | |||
).to.eventually.be.rejectedWith('execa') | |||
}) | |||
|
|||
it('should pass for passed files', async () => { | |||
it.skip('should pass for passed files', async () => { |
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.
There was a regression here and this is failing in #master with:
AssertionError: expected promise to be fulfilled but it was rejected with 'Error: Command failed with exit code 1: /Users/vsantos/gh/tmp/aegir/cli.js dependency-check derp/foo.js\n- Checking dependencies\n✖ Checking dependencies\nCommand failed with exit code 1: dependency-check package.json .aegir.js .aegir.cjs src/**/*.js src/**/*.cjs test/**/*.js test/**/*.cjs benchmarks/**/*.js benchmarks/**/*.cjs utils/**/*.js utils/**/*.cjs !./test/fixtures/**/*.js !./test/fixtures/**/*.cjs derp/foo.js --missing -i @types/* -i @types/*\nFail! Dependencies not listed in package.json: pico'
I skipped this for now, but I will open an issue to get this fixed
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.
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.
Is this failing in master? Passes for me locally and CI for master is green?
The test should pass because it's only supposed to dep check the passed file, but if it's failing it means it's checking more than just the passed file - it's probably worth looking into why this is now broken as it'll almost certainly cause problems on release.
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, it is failing in master too. Did you update the package lock?
Unless it got fixed in the meantime, let me try again
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.
➜ aegir git:(master) nrt
> aegir@34.1.0 test
> node cli.js ts -p check && node cli.js test
Test Node.js
dependency check
1) should pass for passed files
0 passing (520ms)
1 failing
1) dependency check
should pass for passed files:
AssertionError: expected promise to be fulfilled but it was rejected with 'Error: Command failed with exit code 1: /Users/vsantos/work/pl/gh/tmp/aegir/cli.js dependency-check derp/foo.js\n- Checking dependencies\n✖ Checking dependencies\nCommand failed with exit code 1: dependency-check package.json .aegir.js .aegir.cjs src/**/*.js src/**/*.cjs test/**/*.js test/**/*.cjs benchmarks/**/*.js benchmarks/**/*.cjs utils/**/*.js utils/**/*.cjs !./test/fixtures/**/*.js !./test/fixtures/**/*.cjs derp/foo.js --missing -i @types/* -i @types/*\nFail! Dependencies not listed in package.json: pico'
I will reinstall all deps
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.
Actually yeah, removing the lockfile and reinstalling now it's failing. Ugh.
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.
also FYI this happened without any changes in this PR, except for the docs being added. The reason to ignore this here was because this was not related to the PR and I did not want to keep this blocked on an unrelated #master problem
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.
(still happens for me in #master) by removing the package-lock and installing everything with Node 16)
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 think yargs has started merging passed array args with the defaults for that positional. Fix: yargs/yargs#2006
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.
yeah, that sounds like the problem when I was trying to figure it out. Can we move forward with the PR and wait on that to be shipped to unskip the tests?
Shipped in |
This builds up on @achingbrain 's work on #863 with build improvements and full support
This adds:
ipjs
for ESM modulestypes
property transformation, allowing real time ts check in dev and path update for dist folder (TLDR no dist in the path)release
for ESM modules will navigate to the dist to publish its contentOne of the problematic modules in skypack using aegir is
uint8arrays
. It is a CJS module that depends on a ESM first module (multiformats), which makes skypack to get bad dependency paths.I tested this out shipping
uint8arrays
achingbrain/uint8arrays#22 PR and everything working smoothly 🎉Original release: https://codepen.io/vascosantos/pen/KKmXoPV?editors=0011
Scoped release using
aegir
: https://codepen.io/vascosantos/pen/bGWoONq?editors=0011(see browser built in console for errors)