-
Notifications
You must be signed in to change notification settings - Fork 58
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
Changes from 9 commits
09da460
ab5f546
6392861
1938441
2892995
4b23e2e
bcbe58e
2f08d66
be6b4ea
7f41463
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package-lock.json | ||
yarn.lock | ||
node_modules | ||
/node_modules | ||
/actions/bundle-size/node_modules | ||
/coverage | ||
/dist | ||
/docs | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
** | ||
!/dist |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
# Just enough docker until github gets a new node16 runner | ||
# see: https://github.com/actions/runner/issues/772 | ||
FROM node:16-alpine | ||
WORKDIR /usr/src/app | ||
COPY dist/index.js . | ||
CMD [ "node", "index.js" ] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
# ESM support | ||
|
||
## Setup | ||
|
||
`aegir` leverages [ipjs](https://github.com/mikeal/ipjs) to output a build with `cjs` and `esm` for maximum compatibility. The general guidelines for writing a module in `esm` are detailed on the `ipjs` README. `aegir` will automatically identify a `esm` repo by the `module` property in `package.json`. | ||
|
||
## Electron testing | ||
|
||
Electron does [not support ESM](https://github.com/electron/electron/issues/21457) at the time of writing. When writing a module using ESM, we need to compile the tests to `cjs` and rely on them. For generating the build including the tests: | ||
|
||
```bash | ||
aegir build --esm-tests | ||
``` | ||
|
||
## Lerna Monorepo | ||
|
||
When using a lerna monorepo, local dependencies are symlinked by lerna on install. This means that an `esm` module will not use the resulting `dist` folder as symlink. This can become a problem if we are testing the `cjs` build of a module. | ||
|
||
To work around the above problem, we can use `publishConfig.directory = "dist"` in `package.json` to notice lerna about the symlink path. After running the `aegir build` command, it is necessary to run `lerna link` to update the symlinks. | ||
|
||
## Release | ||
|
||
When releasing an `esm` module, the published content will be the generated `dist` folder content, as indicated by [ipjs](https://github.com/mikeal/ipjs). | ||
|
||
## Examples | ||
|
||
TODO: List examples when merged (`ipfs-unixfs`, `uint8arrays`) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
/* eslint-env mocha */ | ||
'use strict' | ||
|
||
const { expect } = require('../utils/chai') | ||
const execa = require('execa') | ||
const { copy, existsSync } = require('fs-extra') | ||
const { join } = require('path') | ||
const bin = require.resolve('../') | ||
const tempy = require('tempy') | ||
|
||
describe('build', () => { | ||
describe('esm', () => { | ||
let projectDir = '' | ||
|
||
before(async () => { | ||
projectDir = tempy.directory() | ||
|
||
await copy(join(__dirname, 'fixtures', 'esm', 'an-esm-project'), projectDir) | ||
}) | ||
|
||
it('should build an esm project', async function () { | ||
this.timeout(20 * 1000) // slow ci is slow | ||
|
||
await execa(bin, ['build'], { | ||
cwd: projectDir | ||
}) | ||
|
||
expect(existsSync(join(projectDir, 'dist', 'esm'))).to.be.true() | ||
expect(existsSync(join(projectDir, 'dist', 'cjs'))).to.be.true() | ||
|
||
const module = require(join(projectDir, 'dist')) | ||
|
||
expect(module).to.have.property('useHerp').that.is.a('function') | ||
expect(module).to.have.property('useDerp').that.is.a('function') | ||
}) | ||
}) | ||
}) |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
{ | ||
"name": "an-esm-project", | ||
"version": "1.0.0", | ||
"description": "", | ||
"main": "src/index.js", | ||
"type": "module", | ||
"scripts": { | ||
"test": "echo \"Error: no test specified\" && exit 1" | ||
}, | ||
"author": "", | ||
"license": "ISC", | ||
"eslintConfig": { | ||
"extends": "ipfs", | ||
"parserOptions": { | ||
"sourceType": "module" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
import herp from 'a-cjs-dep' | ||
import derp from 'an-esm-dep' | ||
|
||
export const useHerp = () => { | ||
herp() | ||
} | ||
|
||
export const useDerp = () => { | ||
derp() | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
'use strict' | ||
|
||
require('./build') | ||
require('./lint') | ||
require('./fixtures') | ||
require('./dependants') | ||
|
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:
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.
#870
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.
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?