-
Notifications
You must be signed in to change notification settings - Fork 490
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!: migrate to ESM #2092
feat!: migrate to ESM #2092
Conversation
currently testing esm module import errors with `npm run test:unit -- src/bundles/identity.test.js`
@whizzzkid confirmed working in ipfs-desktop with: # ~/code/work/protocol.ai/ipfs/webui
npm run build
cp -r build/ ../ipfs-desktop/assets/webui/
# ~/code/work/protocol.ai/ipfs/ipfs-desktop
npm run start
|
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.
Tested this with symlink
ln -s ipfs-webui/build ipfs-desktop/assets/webui
# ipfs-desktop/assets/webui -> ../../ipfs-webui/build
Seems to work well 🚀, some non-blocking nits.
Also, @SgtPooki maybe CI can publish a version of desktop using the webui, for ease of testing in the future?
os: require.resolve('./src/webpack-fallbacks/os'), | ||
path: require.resolve('./src/webpack-fallbacks/path') | ||
stream: 'stream-browserify', | ||
os: 'os-browserify/browser', |
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.
does this need to be browser?
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.
in all honesty, yes it does, doesn't it? We don't really have any instance of a backend web-server for webui. Even in ipfs-desktop, it's "serving" the content from assets/webui
as a basic webpage using https://www.electronjs.org/docs/latest/api/browser-window and https://www.electronjs.org/docs/latest/api/web-contents
So in all honesty, this is a very lazy way to not split up the build deps from the runtime deps, but we're not quite there yet.
moduleNameMapper: { | ||
...config.moduleNameMapper, | ||
'multiformats/basics': '<rootDir>/node_modules/multiformats/cjs/src/basics.js', | ||
'multiformats/bases/(.*)$': '<rootDir>/node_modules/multiformats/cjs/src/bases/$1.js' |
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.
optional perf, check if not the end $
instead of check if element is one of everything-set .
'multiformats/bases/([^$]+)$': '<rootDir>/node_modules/multiformats/cjs/src/bases/$1.js'
we should at least check the size +
[1...inf] instead of *
[0...inf]
'multiformats/bases/(.+)$': '<rootDir>/node_modules/multiformats/cjs/src/bases/$1.js'
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 really shouldn't be any performance difference, and if anything, it should be more performant with the greedy match. we already know the size is > 0 because of the forward-slash in front of X
in imports of multiformats/bases/X
, and we want to replace everything, so the regex engine doesn't have to double/triple check endings/beginnings/counts at all.. just.. get everything.
I put together a quick hyperfine test by doing
jest: (config) => {
let mfBases = 'multiformats/bases/(.*)$'
if (process.env.MFBASE === '1') {
mfBases = 'multiformats/bases/(.+)$'
} else if (process.env.MFBASE === '2') {
mfBases = 'multiformats/bases/([^$]+)$'
}
/**
* @type {import('jest').Config}
*/
return ({
...config,
setupFiles: [...config.setupFiles, 'fake-indexeddb/auto'],
moduleNameMapper: {
...config.moduleNameMapper,
'multiformats/basics': '<rootDir>/node_modules/multiformats/cjs/src/basics.js',
[mfBases]: '<rootDir>/node_modules/multiformats/cjs/src/bases/$1.js'
}
})
}
and running
╰─ ✔ ❯ hyperfine 'MFBASE=0 npm run test:unit' 'MFBASE=1 npm run test:unit' 'MFBASE=2 npm run test:unit'
Benchmark 1: MFBASE=0 npm run test:unit
Time (mean ± σ): 5.429 s ± 0.104 s [User: 3.829 s, System: 1.008 s]
Range (min … max): 5.232 s … 5.587 s 10 runs
Benchmark 2: MFBASE=1 npm run test:unit
Time (mean ± σ): 17.498 s ± 24.296 s [User: 4.158 s, System: 1.064 s]
Range (min … max): 5.483 s … 63.602 s 10 runs
Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet PC without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
Benchmark 3: MFBASE=2 npm run test:unit
Time (mean ± σ): 5.675 s ± 0.114 s [User: 3.911 s, System: 1.042 s]
Range (min … max): 5.548 s … 5.919 s 10 runs
Summary
'MFBASE=0 npm run test:unit' ran
1.05 ± 0.03 times faster than 'MFBASE=2 npm run test:unit'
3.22 ± 4.48 times faster than 'MFBASE=1 npm run test:unit'
npm/jest actually stalls out on MFBASE=1, below is the output when running MFBASE=1 npm run test:unit
directly.
Jest did not exit one second after the test run has completed.
This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue.
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.
MFBASE=0 actually has two additional checks for the process.env.MFBASE whereas the other two do not, so it's saying something that it's still faster.
Either way, optimizing regex here is.. tedious =P
"test:unit": "cross-env NODE_OPTIONS=--experimental-vm-modules react-app-rewired-esm test --env=jsdom --runInBand --watchAll=false", | ||
"test:unit:coverage": "cross-env NODE_OPTIONS=--experimental-vm-modules react-app-rewired-esm test --env=jsdom --runInBand --watchAll=false --coverageDirectory='./.nyc_output' --coverage", | ||
"test:unit:watch": "cross-env NODE_OPTIONS=--experimental-vm-modules react-app-rewired-esm test --env=jsdom", |
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.
in the future we should look into dotenv
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.
self review
- name: Install playwright browsers | ||
run: npx playwright install --with-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.
This is basically a no-op if things are up to date, but required if any playwright updates are done (which were in this PR)
/** @type {import('@storybook/core-common').StorybookConfig} */ | ||
const storybookConfig = { | ||
core: { | ||
builder: 'webpack5' | ||
}, | ||
reactOptions: { | ||
legacyRootApi: true | ||
legacyRootApi: false |
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.
No need to disable this anymore. https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#react18-new-root-api
storyStoreV7: true, | ||
babelModeV7: true |
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.
getting ready for v7
}, | ||
extensions: [ | ||
...config.resolve.extensions, | ||
'dist/esm/index.js' |
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.
fixes some issues with imports that don't have proper export fields; or our tools (playwright/storybook) that don't handle ESM packages properly.
@@ -0,0 +1,265 @@ | |||
diff --git a/node_modules/eslint-plugin-import/config/recommended-esm.js b/node_modules/eslint-plugin-import/config/recommended-esm.js |
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.
will be able to remove when import-js/eslint-plugin-import#2701 is merged
@@ -8,14 +8,33 @@ | |||
* Run the tests with | |||
* KUBO_PORT_2033_TEST=5001 npm run test:unit -- --runTestsByPath "test/kubo-webtransport.test.js" --env=./custom-jest-env.js | |||
*/ | |||
import ipfsHttpModule from 'ipfs-http-client' | |||
import { createController } from 'ipfsd-ctl' | |||
describe.skip('identity.js', function () { |
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.
currently skipping this due to some issues, but this was only added to address concerns with #2033 and isn't really a valid test.
// TODO: We should provide a way to log these errors when debugging | ||
// if (['development', 'test'].includes(process.env.REACT_APP_ENV)) { | ||
// console.error(e) | ||
// } |
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.
will create an issue to address this
lookup: (_, address) => { | ||
if (address === '5.5.5.5') throw new Error() | ||
return `${address}+looked-up` | ||
import createPeersLocationBundle from './peer-locations.js' |
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.
had to change this test to handle the removal of module mocks
import { withKnobs, text, number, color } from '@storybook/addon-knobs' | ||
import React from 'react' | ||
|
||
const requireContext = require.context('.', true, /\.js?$/) | ||
const modules = requireContext.keys().filter((c) => !c.includes('.stories') && !c.includes('index.js')) | ||
const icons = modules.map((m) => ({ | ||
name: m.replace('./', '').replace('.js', ''), | ||
Icon: requireContext(m).default | ||
import * as iconImports from './index.js' | ||
const icons = Object.keys(iconImports).map((m) => ({ | ||
name: m, | ||
Icon: iconImports[m] | ||
})) | ||
|
||
const filterByTextQuery = (icon) => { | ||
const searchQuery = text('Search', '') | ||
return icon.name.includes(searchQuery) | ||
const filterByTextQuery = (icon, searchQuery) => { | ||
return icon.name.toLowerCase().includes(searchQuery.toLowerCase()) | ||
} |
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.
had to change how we were loading icons because require.context isn't available. we export all icons from index.js, so importing them all from there.
## [3.0.0](v2.22.0...v3.0.0) (2023-04-24) CID `bafybeic4gops3d3lyrisqku37uio33nvt6fqxvkxihrwlqsuvf76yln4fm` --- ### ⚠ BREAKING CHANGES * migrate to ESM (#2092) ### Features * ipfs-http-client -> kubo-rpc-client ([#2098](#2098)) ([5e53c79](5e53c79)), closes [#issuecomment-1426219633](https://github.com/ipfs/ipfs-webui/issues/issuecomment-1426219633) [/github.com//issues/2079#issuecomment-1426337490](https://github.com/ipfs//github.com/ipfs/ipfs-webui/issues/2079/issues/issuecomment-1426337490) * migrate to ESM ([#2092](#2092)) ([58a737c](58a737c)) ### Bug Fixes * e2e/explore.test.js succeeds in offline mode ([#2109](#2109)) ([a5e9ac6](a5e9ac6)) * ko language falls back to ko-KR ([#2102](#2102)) ([3369800](3369800)) * semantic release custom notes import ([#2113](#2113)) ([2f9f306](2f9f306)) ### Trivial Changes * add NetworkTraffic.stories.js ([#2094](#2094)) ([7a3bf46](7a3bf46)) * pull new translations ([#2101](#2101)) ([cbabac3](cbabac3)) * pull new translations ([#2104](#2104)) ([4a691a2](4a691a2)) * Pull transifex translations ([#2088](#2088)) ([a5b8a1c](a5b8a1c)) * Pull transifex translations ([#2091](#2091)) ([d209863](d209863)) * Pull transifex translations ([#2099](#2099)) ([1cf2fe7](1cf2fe7)) * Pull transifex translations ([#2111](#2111)) ([57d9b63](57d9b63))
🎉 This PR is included in version 3.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
npm run eslint -- --fix