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

chore: switch to ESM #161

Merged
merged 16 commits into from
Aug 19, 2021
Merged

chore: switch to ESM #161

merged 16 commits into from
Aug 19, 2021

Conversation

vasco-santos
Copy link
Member

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

BREAKING CHANGE: built content includes ESM and CJS

This PR switches the codebase to ESM and exports ESM and CJS code. The main goal is to enable ipfs-unixfs to be built on next generation bundlers like skypack: https://codepen.io/vascosantos/pen/NWjaYqP?editors=0011 This is a problem that has not been fixed in skypack yet. More details: skypackjs/skypack-cdn#171

Given we will need to export ESM and we will, let's get this to circumvent the issue.

Needs:

TODO:

  • add dynamic imports
  • fix types where ignore was added

BREAKING CHANGE: built content includes ESM and CJS
@rvagg
Copy link
Member

rvagg commented Jul 22, 2021

brave man ... you have my 🪓

@achingbrain
Copy link
Member

If we play it right this shouldn't be a breaking change, right? Everything here is named exports already so the public API shouldn't be changed?

@achingbrain
Copy link
Member

That protobuf.js issue is a pain. That and how protobufjs/protobuf.js#1115 has ground to a halt makes me think maybe it was the wrong choice.

@vasco-santos
Copy link
Member Author

If we play it right this shouldn't be a breaking change, right? Everything here is named exports already so the public API shouldn't be changed?

Well yes, but with these kind of things maybe better to be safe?

@vasco-santos vasco-santos force-pushed the chore/switch-to-esm branch from b669091 to f438708 Compare July 23, 2021 12:05
.travis.yml Outdated
@@ -54,11 +54,6 @@ jobs:
script:
- npm run depcheck -- $RUN_SINCE -- -- -p

- stage: check
Copy link
Member Author

Choose a reason for hiding this comment

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

So, we are currently living on a fork ok dependency-check in aegir: https://github.com/ipfs/aegir/blob/master/package.json#L59

I am not aware of the history behind this, but apparently it was only released as non stable. They are working now on version 5 with the next label, but I am not sure if this will be fixed: https://www.npmjs.com/package/dependency-check

The problem with the ESM change is that it seems to believe nothing is being used...

Copy link
Member

Choose a reason for hiding this comment

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

Looks like dependency-check uses detective to find which modules are required. Probably needs to use detective-es6 when parsing ESM modules.

Copy link
Member

Choose a reason for hiding this comment

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

Actually I think we just need to pass the --detective arg to dependency-check with a path to detective-es6

Copy link
Member

Choose a reason for hiding this comment

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

@vasco-santos
Copy link
Member Author

That protobuf.js issue is a pain. That and how protobufjs/protobuf.js#1115 has ground to a halt makes me think maybe it was the wrong choice.

yeah, I pinged them on protobufjs/protobuf.js#1621 to see. For now, we can probably get a postbuild script that goes through the file and does this change... But it is annoying and error prone. Other solution is to temporarily not create the protobuf file on every build. Let me know what you prefer @achingbrain

@vasco-santos vasco-santos force-pushed the chore/switch-to-esm branch from 0c5b4ff to ca57bf8 Compare July 23, 2021 14:08
@vasco-santos vasco-santos force-pushed the chore/switch-to-esm branch from ca57bf8 to 5616c33 Compare July 23, 2021 14:15
@vasco-santos
Copy link
Member Author

Tests are only failing in electron-main now. I think we will need to transpile for this per jprichardson/electron-mocha#180 but I will dive deeper.

Meanwhile, it would be great to have your eyes on this PR @achingbrain

@vasco-santos vasco-santos force-pushed the chore/switch-to-esm branch from 84736e4 to 6b5c08c Compare July 29, 2021 09:54
const importResolver = async (key) => {
switch (key) {
case dagPb.code:
return (await (import('./unixfs-v1/index.js'))).default
Copy link
Member

Choose a reason for hiding this comment

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

Could just import these as normal, I don't think there's a pressing need for a dynamic import.

},
symlink: (cid, node, unixfs, path, resolve, depth, blockstore) => {
return () => []
const importContentExporters = async (key) => {
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as the resolvers above, we're just introducing async for no reason.

flat: require('./flat'),
balanced: require('./balanced'),
trickle: require('./trickle')
const importDagBuilder = async (key) => {
Copy link
Member

Choose a reason for hiding this comment

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

Same as resolvers, these can be imported as normal, no need for a dynamic import.


/**
* @typedef {import('./dir')} Dir
* @typedef {import('./dir').default} Dir
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, why did this need the .default adding?

Copy link
Member Author

Choose a reason for hiding this comment

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

The type inferred without .default is:

type Dir = typeof import("/Users/vsantos//gh/ipfs/js-ipfs-unixfs/packages/ipfs-unixfs-importer/src/dir")

which makes usage of Dir types to fail with:

Property 'parent' does not exist on type 'typeof import("/Users/vsantos/work/pl/gh/ipfs/js-ipfs-unixfs/packages/ipfs-unixfs-importer/src/dir")'

We either need to export default, given we are currently exporting default in Dir class, or we need to name export and here add the named export

@@ -39,7 +44,7 @@ async function * buildFileBatch (file, blockstore, options) {
if (typeof options.bufferImporter === 'function') {
bufferImporter = options.bufferImporter
} else {
bufferImporter = require('./buffer-importer')
bufferImporter = (await (import('./buffer-importer.js'))).default
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we keep this dynamic import @achingbrain ?

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 are other occurrences, did you just flag the ones you consider that should not be dynamic? or a subset and all dynamic imports should go away?

@vasco-santos vasco-santos force-pushed the chore/switch-to-esm branch from 2c24e3d to 83e27ba Compare July 29, 2021 21:56
@vasco-santos vasco-santos marked this pull request as ready for review August 16, 2021 13:11
@achingbrain achingbrain merged commit 5db8673 into master Aug 19, 2021
@achingbrain achingbrain deleted the chore/switch-to-esm branch August 19, 2021 09:19
achingbrain added a commit that referenced this pull request Aug 19, 2021
Switches the codebase to ESM and exports ESM and CJS code. The main goal is to enable ipfs-unixfs to be built on next generation bundlers like skypack: codepen.io/vascosantos/pen/NWjaYqP?editors=0011 This is a problem that has not been fixed in skypack yet. More details: skypackjs/skypack-cdn#171

Given we will need to export ESM and we will, let's get this to circumvent the issue.

We also need to manually change the generated protobuf code due to protobufjs/protobuf.js#1230

BREAKING CHANGE: ./src/dir-sharded is not in the exports map so cannot be imported

Co-authored-by: Alex Potsides <[email protected]>
github-actions bot pushed a commit that referenced this pull request Apr 26, 2022
## ipfs-unixfs-v1.0.0 (2022-04-26)

### ⚠ BREAKING CHANGES

* ./src/dir-sharded is not in the exports map so cannot be imported

Co-authored-by: Alex Potsides <[email protected]>
* uses new multiformats stack and takes a blockservice instead of the block api

Co-authored-by: Rod Vagg <[email protected]>
Co-authored-by: achingbrain <[email protected]>
* switches to named exports
* types are now included with all unixfs modules
* does not convert input to node Buffers any more, uses Uint8Arrays instead

### Features

* add types ([#114](#114)) ([ca26353](ca26353))

### Bug Fixes

* add pbjs namespace ([#145](#145)) ([dd26b92](dd26b92))
* declare types in .ts files ([#168](#168)) ([76ec6e5](76ec6e5))
* ignore high mode bits passed to constructor ([#53](#53)) ([8e8d83d](8e8d83d))
* ignore undefined values in options ([#173](#173)) ([200dff3](200dff3))
* individual packages can use npm 6 ([#167](#167)) ([2b429cc](2b429cc))
* publish with types in package.json ([#166](#166)) ([0318c98](0318c98))
* remove node globals ([#52](#52)) ([5414412](5414412))
* replace node buffers with uint8arrays ([#69](#69)) ([8a5aed2](8a5aed2)), closes [#66](#66)
* types with ipjs build ([#165](#165)) ([fea85b5](fea85b5))
* use @ipld/dag-pb instead of ipld-dag-pb ([#116](#116)) ([bab1985](bab1985))

### Trivial Changes

* add travis file and configure build scripts ([5a25c87](5a25c87))
* consolidate .gitignore files ([b05e468](b05e468))
* declare interface types in .d.ts file ([#122](#122)) ([eaa8449](eaa8449))
* dep updates ([cf9480b](cf9480b))
* **deps-dev:** bump aegir from 26.0.0 to 28.1.0 ([#86](#86)) ([87541c7](87541c7))
* **deps-dev:** bump aegir from 28.2.0 to 29.2.2 ([#101](#101)) ([010ab47](010ab47))
* exclude docs and tests from npm package ([63b8ba0](63b8ba0))
* move files into packages folder ([943be9d](943be9d))
* publish ([5203595](5203595))
* publish ([0f9092e](0f9092e))
* publish ([2713329](2713329))
* publish ([35e2059](35e2059))
* publish ([137a4ad](137a4ad))
* publish ([f173850](f173850))
* publish ([2ea467f](2ea467f))
* publish ([dedbd82](dedbd82))
* publish ([dc2d400](dc2d400))
* publish ([27d57df](27d57df))
* publish ([5ccac2f](5ccac2f))
* publish ([172548b](172548b))
* publish ([9a2b5f2](9a2b5f2))
* publish ([e57ba16](e57ba16))
* publish ([9e8f077](9e8f077))
* publish ([22e29bb](22e29bb))
* publish ([dabbb48](dabbb48))
* publish ([32e5165](32e5165))
* publish ([5d3f4bd](5d3f4bd))
* publish ([49c8c54](49c8c54))
* publish ([db2c878](db2c878))
* publish ([9237250](9237250))
* remove changes from readme ([7c727ef](7c727ef))
* remove node buffers from runtime code ([#66](#66)) ([db60a42](db60a42))
* remove redundant test files ([3078608](3078608))
* small readme change ([f45436c](f45436c))
* swap to prereleaseOnly ([efb01ac](efb01ac))
* switch to auto-release ([#208](#208)) ([99386e6](99386e6))
* switch to ESM ([#161](#161)) ([819267f](819267f)), closes [skypackjs/skypack-cdn#171](skypackjs/skypack-cdn#171)
* tighten up input types ([#133](#133)) ([47f295b](47f295b))
* update build scripts ([37d96ee](37d96ee))
* update deps ([#144](#144)) ([f5f5fe4](f5f5fe4))
* update deps ([#78](#78)) ([2bf5d07](2bf5d07))
* update lockfiles ([9d11252](9d11252))
* update package.json scripts and readmes ([bda3717](bda3717))
* update readme ([a012f22](a012f22))
* update readme ([7f93da1](7f93da1))
* update readmes ([#188](#188)) ([273a141](273a141))
* upgrade deps ([3a43e92](3a43e92))
* use npm 7 workspaces instead of lerna bootstrap ([#120](#120)) ([1ceb097](1ceb097))
achingbrain added a commit that referenced this pull request May 27, 2022
We have `typesVersions` to allow deep imports from these modules, but
they cause incorrect paths to be detected by tsc (see #214).

Since #161 we control exports using the exports map, and we only export
the root from each module so `typesVersions` isn't necessary.

Since it's causing problems and we don't need it, remove it.  We can
revisit once we publish ESM-only but right now this fixes up the types
tsc generates.

Fixes #214
achingbrain added a commit that referenced this pull request May 27, 2022
We have `typesVersions` to allow deep requires from these modules, but
they cause incorrect paths to be detected by tsc (see #214).

Since #161 we control exports using the exports map, and we only export
the root from each module so deep requires are disallowed and consequently
`typesVersions` isn't necessary.

Since it's causing problems and we don't need it, remove it.  We can
revisit once we publish ESM-only but right now this fixes up the types
tsc generates.

Fixes #214
github-actions bot pushed a commit that referenced this pull request May 27, 2022
## [ipfs-unixfs-v6.0.9](ipfs-unixfs-v6.0.8...ipfs-unixfs-v6.0.9) (2022-05-27)

### Bug Fixes

* remove typesVersions from package.json ([#219](#219)) ([465670e](465670e)), closes [#214](#214) [#161](#161) [#214](#214)

### Trivial Changes

* update dep-check command ([#221](#221)) ([5802bd3](5802bd3))
github-actions bot pushed a commit that referenced this pull request May 27, 2022
## [ipfs-unixfs-importer-v9.0.10](ipfs-unixfs-importer-v9.0.9...ipfs-unixfs-importer-v9.0.10) (2022-05-27)

### Bug Fixes

* remove typesVersions from package.json ([#219](#219)) ([465670e](465670e)), closes [#214](#214) [#161](#161) [#214](#214)

### Trivial Changes

* update dep-check command ([#221](#221)) ([5802bd3](5802bd3))
github-actions bot pushed a commit that referenced this pull request May 27, 2022
## [ipfs-unixfs-exporter-v7.0.11](ipfs-unixfs-exporter-v7.0.10...ipfs-unixfs-exporter-v7.0.11) (2022-05-27)

### Bug Fixes

* remove typesVersions from package.json ([#219](#219)) ([465670e](465670e)), closes [#214](#214) [#161](#161) [#214](#214)

### Trivial Changes

* update dep-check command ([#221](#221)) ([5802bd3](5802bd3))
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.

3 participants