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

[WIP] feat: support aegir.ts config file #994

Closed
wants to merge 12 commits into from

Conversation

SgtPooki
Copy link
Member

@SgtPooki SgtPooki commented Jun 9, 2022

also tested against ipfs-shipyard/pinning-service-compliance by running

npm run build
npm pack
mv aegir-37.0.18.tgz ~/Downloads
cd /Users/sgtpooki/code/work/protocol.ai/ipfs/pinning-service-compliance/pinning-service-compliance
npm install -S ~/Downloads/aegir-37.0.18.tgz

# converted .aegir.js to .aegir.ts, then...

> npm run test

> @ipfs-shipyard/[email protected] test
> aegir lint

[17:16:29] eslint [started]
=============

WARNING: You are currently running a version of TypeScript which is not officially supported by @typescript-eslint/typescript-estree.

You may find that it works just fine, or you may not.

SUPPORTED TYPESCRIPT VERSIONS: >=3.3.1 <4.5.0

YOUR TYPESCRIPT VERSION: 4.6.4

Please only submit bug reports when using the officially supported version.

=============
[17:16:31] eslint [completed]
[17:16:31] tsc [started]
[17:16:32] tsc [completed]

> npm run build

> @ipfs-shipyard/[email protected] build
> aegir build

[17:17:50] tsc [started]
[17:17:51] tsc [completed]
[17:17:51] esbuild [started]
[17:17:52] esbuild [completed]

@SgtPooki SgtPooki linked an issue Jun 9, 2022 that may be closed by this pull request
@SgtPooki SgtPooki self-assigned this Jun 9, 2022
@SgtPooki SgtPooki requested review from achingbrain and hugomrdias and removed request for achingbrain June 9, 2022 00:21
README.md Outdated Show resolved Hide resolved
src/config/user.js Outdated Show resolved Hide resolved
@achingbrain achingbrain changed the title 943 aegir should support aegir.ts config file feat: support aegir.ts config file Jun 9, 2022
.aegir.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/config/user.js Outdated Show resolved Hide resolved
src/config/user.js Outdated Show resolved Hide resolved
@SgtPooki
Copy link
Member Author

Added support for no-extension files, e.g. .aegir that attempt to load as ESM and then TS.

.aegir.ts Outdated
@@ -0,0 +1,7 @@
const options: import('./src/types').PartialOptions = {
Copy link
Member

Choose a reason for hiding this comment

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

That's interesting, I didn't know you could do inline imports of types like this.

That said, is it better than import type { Foo } from './bar.js'?

Also, I see there's no extension on ./src/types - does this mean this file gets compiled to CJS in the background? What happens if you try to import an ESM-only module in your .aegir file?

Copy link
Member Author

Choose a reason for hiding this comment

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

let me look into that

Copy link
Member Author

Choose a reason for hiding this comment

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

That said, is it better than import type { Foo } from './bar.js'?

Nope! definitely not. fixing this.

Also, I see there's no extension on ./src/types - does this mean this file gets compiled to CJS in the background?

Not sure, but fixing this.

What happens if you try to import an ESM-only module in your .aegir file?

updated to

import type { PartialOptions } from './src/types.js'
import { isWritableStream } from 'is-stream'
console.log('isWritableStream: ', isWritableStream)

const options: PartialOptions = {
  docs: {
    entryPoint: 'utils'
  }
}

export default options

and ran npm run lint:

npm run lint

> [email protected] lint
> node src/index.js lint

Unexpected error while loading your config file
Unexpected error loading aegir config
/Users/sgtpooki/code/work/protocol.ai/ipfs/aegir/node_modules/ts-import/cache/Users/sgtpooki/code/work/protocol.ai/ipfs/aegir/.aegir.js:3
var is_stream_1 = require("is-stream");
                  ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/sgtpooki/code/work/protocol.ai/ipfs/aegir/node_modules/is-stream/index.js from /Users/sgtpooki/code/work/protocol.ai/ipfs/aegir/node_modules/ts-import/cache/Users/sgtpooki/code/work/protocol.ai/ipfs/aegir/.aegir.js not supported.
Instead change the require of index.js in /Users/sgtpooki/code/work/protocol.ai/ipfs/aegir/node_modules/ts-import/cache/Users/sgtpooki/code/work/protocol.ai/ipfs/aegir/.aegir.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/Users/sgtpooki/code/work/protocol.ai/ipfs/aegir/node_modules/ts-import/cache/Users/sgtpooki/code/work/protocol.ai/ipfs/aegir/.aegir.js:3:19)
    at /Users/sgtpooki/code/work/protocol.ai/ipfs/aegir/node_modules/ts-import/dist/main.js:34:55
    at async Object.load (/Users/sgtpooki/code/work/protocol.ai/ipfs/aegir/node_modules/ts-import/dist/main.js:34:20)
    at async loadTs (file:///Users/sgtpooki/code/work/protocol.ai/ipfs/aegir/src/config/user.js:61:11)
    at async Object.search (/Users/sgtpooki/code/work/protocol.ai/ipfs/aegir/node_modules/lilconfig/dist/index.js:126:37)
    at async config (file:///Users/sgtpooki/code/work/protocol.ai/ipfs/aegir/src/config/user.js:110:26)
    at async main (file:///Users/sgtpooki/code/work/protocol.ai/ipfs/aegir/src/index.js:48:22) {
  code: 'ERR_REQUIRE_ESM'
}

Node.js v17.4.0

Copy link
Member Author

Choose a reason for hiding this comment

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

seems like ts-import doesn't support esm, which would break things

Copy link
Member Author

Choose a reason for hiding this comment

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

adding a test for this..

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is what I thought might happen. It might be worth opening an issue on the ts-import repo about it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like this was fixed on Dec 5: radarsu/ts-import#24 (comment) with version 4.0.0-beta.10. I won't be able to get back to this PR for a bit, but documenting this

@SgtPooki SgtPooki force-pushed the 943-aegir-should-support-aegirts-config-file branch from 265a8bf to 2cbe5c6 Compare June 23, 2022 21:13
@SgtPooki
Copy link
Member Author

pushed the new tests, but it's currently failing with the error specified above.

@SgtPooki SgtPooki marked this pull request as draft January 19, 2023 20:43
@SgtPooki SgtPooki changed the title feat: support aegir.ts config file [WIP] feat: support aegir.ts config file Jan 19, 2023
@SgtPooki
Copy link
Member Author

closing this for now because I don't have the bandwidth to take this on

@SgtPooki SgtPooki closed this Feb 13, 2023
@SgtPooki SgtPooki deleted the 943-aegir-should-support-aegirts-config-file branch February 13, 2023 19:34
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.

aegir should support .aegir.ts config file
2 participants