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

feat: build esm modules using ipjs #865

Merged
merged 10 commits into from Aug 14, 2021

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Jul 22, 2021

This builds up on @achingbrain 's work on #863 with build improvements and full support

This adds:

One 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)

@vasco-santos
Copy link
Member Author

Probably ipjs should take care of the types transformation per mikeal/ipjs#10 , but it might not be a concern of ipjs to consider setups like these

test/build.js Outdated
const tempy = require('tempy')

describe('build', () => {
if (process.platform !== 'win32') {
Copy link
Member Author

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

Copy link
Member Author

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?

Copy link
Member

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?

Copy link
Member Author

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

@achingbrain
Copy link
Member

achingbrain commented Jul 27, 2021

We need to generate types from /src into /types, then copy /types and /src into /dist otherwise the paths in the ts.map files in the /types dir get all messed up (since they are generated from /src and contain relative paths) and click-through breaks.

We can't generate types from the files in /dist because they are transpiled which can have unexpected results (see also, fix).

@vasco-santos vasco-santos force-pushed the feat/build-esm-modules-follow-up branch 23 times, most recently from 7db225c to a9d879d Compare July 28, 2021 17:12
@vasco-santos vasco-santos force-pushed the feat/build-esm-modules-follow-up branch from ff580fe to 2892995 Compare July 29, 2021 16:23
@vasco-santos vasco-santos force-pushed the feat/build-esm-modules-follow-up branch from d4e519e to 4b23e2e Compare July 29, 2021 22:01
@vasco-santos vasco-santos requested review from achingbrain and removed request for achingbrain July 29, 2021 22:08
@vasco-santos vasco-santos force-pushed the feat/build-esm-modules-follow-up branch 6 times, most recently from 6e6d148 to cf254fe Compare August 1, 2021 00:55
@vasco-santos vasco-santos force-pushed the feat/build-esm-modules-follow-up branch from 7698a49 to 8501679 Compare August 11, 2021 14:22
@vasco-santos vasco-santos force-pushed the feat/build-esm-modules-follow-up branch from 8501679 to 2f08d66 Compare August 11, 2021 14:27
@@ -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 () => {
Copy link
Member Author

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member Author

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)

Copy link
Member

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

Copy link
Member Author

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?

@achingbrain achingbrain merged commit fa70ff7 into master Aug 14, 2021
@achingbrain achingbrain deleted the feat/build-esm-modules-follow-up branch August 14, 2021 07:31
@achingbrain
Copy link
Member

Shipped in aegir@35.0.0

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.

None yet

2 participants