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!: migrate to ESM #2092

Merged
merged 38 commits into from
Feb 9, 2023
Merged
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
50ba488
feat!: migrate to kubo-rpc-client
SgtPooki Jan 14, 2023
d5ce138
tmp: revert me.. just showing work
SgtPooki Jan 16, 2023
64321da
Merge branch 'main' into 2079-feat-replace-ipfs-http-client-with-kubo…
SgtPooki Feb 1, 2023
8807176
chore: set type=module
SgtPooki Feb 1, 2023
5e41cac
chore: use react-app-rewired-esm
SgtPooki Feb 1, 2023
ec82663
chore: use esm import update rule
SgtPooki Feb 1, 2023
5aa8035
chore: run `npm run eslint -- --fix`
SgtPooki Feb 1, 2023
ac3460d
chore: lint
SgtPooki Feb 1, 2023
977c5a6
chore: getting tests working
SgtPooki Feb 1, 2023
c1529e3
chore: fix pqueue constructor error
SgtPooki Feb 1, 2023
039c430
fix: peer-locations.test.js
SgtPooki Feb 3, 2023
e1ef368
tmp: trying to fix identity.test.js
SgtPooki Feb 3, 2023
e1f0c9b
fix: AsyncRequestLoader.test.js
SgtPooki Feb 3, 2023
5c2563a
chore: fix e2e/setup/ipfs-backend.js
SgtPooki Feb 3, 2023
3bb96fc
fix: npm start loads the webui
SgtPooki Feb 4, 2023
b2cb7ab
fix: build,start,test succeeding
SgtPooki Feb 6, 2023
25eb3b1
fix: some e2e tests passing
SgtPooki Feb 6, 2023
72cc358
fix: e2e/files.test.js
SgtPooki Feb 6, 2023
e4f2cbb
fix: e2e/ipns.test.js
SgtPooki Feb 6, 2023
7baa706
fix: e2e/peers.test.js
SgtPooki Feb 6, 2023
cc8bae0
fix: e2e/remote-api.test.js
SgtPooki Feb 6, 2023
4e27c4f
fix: e2e/settings.test.js
SgtPooki Feb 6, 2023
69f1dd1
fix: npm run build-storybook
SgtPooki Feb 7, 2023
1c62aba
fix: storybook build & test
SgtPooki Feb 7, 2023
c8dbbb5
fix(storybook): icons.stories.js
SgtPooki Feb 7, 2023
1d295ef
fix: storybook and e2e tests both passing
SgtPooki Feb 7, 2023
2db8cb7
fix: lint
SgtPooki Feb 7, 2023
65846ee
fix: pull package-lock.json from main and npm i
SgtPooki Feb 7, 2023
39137a7
chore: use commit hash for react-app-rewired-esm
SgtPooki Feb 7, 2023
3fb7238
chore: fix eslint error on github CI
SgtPooki Feb 7, 2023
68c0136
chore: use published @sgtpooki/react-app-rewired-esm
SgtPooki Feb 7, 2023
bb6f391
fix: npm run test:unit:coverage
SgtPooki Feb 7, 2023
05e5c19
Merge branch 'main' into 2090-migrate-to-esm
SgtPooki Feb 7, 2023
c918b83
chore: fix github CI for e2e tests
SgtPooki Feb 8, 2023
785771c
Merge branch 'main' into 2090-migrate-to-esm
SgtPooki Feb 8, 2023
32c2049
chore: clean up debugging code
SgtPooki Feb 8, 2023
5218305
chore: address self-review PR comments
SgtPooki Feb 8, 2023
8b64be5
chore: address PR comment
SgtPooki Feb 9, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions .eslintrc.js → .eslintrc.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,20 @@ module.exports = {
}
},
extends: ['react-app', 'standard', 'plugin:jsx-a11y/recommended'],
plugins: ['jsx-a11y', 'storybook'],
plugins: ['jsx-a11y', 'storybook', 'import'],
// ignore .ts files because it fails to parse it.
ignorePatterns: 'src/**/*.ts',
rules: {
'import/esm-extensions': 'error',
'react/prop-types': [0, { ignore: ['className'], customValidators: [], skipUndeclared: true }] // TODO: set this rule to error when all issues are resolved.
},
overrides: [
{
files: ['src/**/*.stories.js'],
excludedFiles: '*.test.js',
rules: {
'import/no-anonymous-default-export': 'off'
'import/no-anonymous-default-export': 'off',
'import/esm-extensions': 'error'
}
}
]
Expand Down
3 changes: 3 additions & 0 deletions .github/workflows/test-e2e.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ jobs:
- name: Install dependencies
run: npm ci --prefer-offline --no-audit --progress=false --cache ${{ github.workspace }}/.cache/npm

- name: Install playwright browsers
run: npx playwright install --with-deps

Comment on lines +37 to +39
Copy link
Member Author

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)

# This is required to ensure that our code is instrumented with coverage details
- name: Run test build
run: npm run test:build
Expand Down
28 changes: 24 additions & 4 deletions .storybook/main.js → .storybook/main.cjs
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
/* eslint-disable import/esm-extensions */
/**
* @file StoryBook configuration file
* @see https://github.com/storybookjs/storybook/blob/master/MIGRATION.md#from-version-52x-to-53x
*/

const webpack = require('webpack')

const { webpack: webpackOverride } = require('../config-overrides')

/** @type {import('@storybook/core-common').StorybookConfig} */
const storybookConfig = {
core: {
builder: 'webpack5'
},
reactOptions: {
legacyRootApi: true
legacyRootApi: false
Copy link
Member Author

Choose a reason for hiding this comment

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

},
stories: [
'../src/**/*.stories.@(ts|js|tsx|jsx)'
Expand All @@ -35,11 +34,32 @@ const storybookConfig = {
],
features: {
postcss: false,
storyStoreV7: true
storyStoreV7: true,
babelModeV7: true
Comment on lines +37 to +38
Copy link
Member Author

Choose a reason for hiding this comment

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

getting ready for v7

},
webpackFinal: async (config) => {
// console.log('config: ', config)
SgtPooki marked this conversation as resolved.
Show resolved Hide resolved
const { webpack: webpackOverride } = (await import('../config-overrides.js')).default

config.module.rules.push({
test: /\.(m?js)$/,
type: 'javascript/auto',
resolve: {
fullySpecified: false
}
})
return webpackOverride({
...config,
resolve: {
...config.resolve,
alias: {
...config.resolve.alias
},
extensions: [
...config.resolve.extensions,
'dist/esm/index.js'
Copy link
Member Author

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.

]
},
// @see https://github.com/storybookjs/storybook/issues/18276#issuecomment-1137101774
plugins: config.plugins.map(plugin => {
if (plugin.constructor.name === 'IgnorePlugin') {
Expand Down
3 changes: 3 additions & 0 deletions .storybook/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"type": "module"
}
12 changes: 9 additions & 3 deletions .storybook/preview.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import React from 'react'
import { Provider } from 'redux-bundler-react'
import { I18nextProvider } from 'react-i18next'
import { DndProvider } from 'react-dnd'
import 'react-virtualized/styles.css'
import '../src/index.css'

import getStore from '../src/bundles'
import i18n from '../src/i18n'
import DndBackend from '../src/lib/dnd-backend'
import getStore from '../src/bundles/index.js'
import i18n from '../src/i18n.js'
import DndBackend from '../src/lib/dnd-backend.js'

/**
* @type {import('@storybook/addons').BaseAnnotations}
Expand Down Expand Up @@ -38,3 +39,8 @@ const baseAnnotations = {

export const decorators = baseAnnotations.decorators
export const argTypes = baseAnnotations.argTypes

// module.exports = {
// decorators: baseAnnotations.decorators,
// argTypes: baseAnnotations.argTypes
// }
Copy link
Member Author

Choose a reason for hiding this comment

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

will remove

4 changes: 4 additions & 0 deletions babel.config.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@

module.exports = {
presets: ['@babel/preset-react']
}
45 changes: 37 additions & 8 deletions config-overrides.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
* @see https://github.com/facebook/create-react-app/issues/11756#issuecomment-1184657437
* @see https://alchemy.com/blog/how-to-polyfill-node-core-modules-in-webpack-5
*/
const webpack = require('webpack')
import webpack from 'webpack'

const PURE_ESM_MODULES = [
'ipfs-geoip'
]
Expand Down Expand Up @@ -67,14 +68,13 @@ function modifyBabelLoaderRuleForTest (rules) {
})
}

function webpackOverride(config) {
function webpackOverride (config) {
const fallback = config.resolve.fallback || {}

Object.assign(fallback, {
assert: require.resolve('./src/webpack-fallbacks/assert'),
stream: require.resolve('./src/webpack-fallbacks/stream'),
os: require.resolve('./src/webpack-fallbacks/os'),
path: require.resolve('./src/webpack-fallbacks/path')
stream: 'stream-browserify',
os: 'os-browserify/browser',
Copy link
Contributor

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?

Copy link
Member Author

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.

path: 'path-browserify'
Comment on lines +75 to +77
Copy link
Member Author

Choose a reason for hiding this comment

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

we can import these directly again

})

config.resolve.fallback = fallback
Expand All @@ -87,6 +87,20 @@ function webpackOverride(config) {
])

config.module.rules = modifyBabelLoaderRuleForBuild(config.module.rules)
config.module.rules.push({
test: /\.(js|jsx)$/,
SgtPooki marked this conversation as resolved.
Show resolved Hide resolved
exclude: /(node_modules|bower_components)/,
loader: 'babel-loader',
options: { presets: ['@babel/env', '@babel/preset-react'] }
})

config.module.rules.push({
test: /\.(m?js)$/,
SgtPooki marked this conversation as resolved.
Show resolved Hide resolved
type: 'javascript/auto',
resolve: {
fullySpecified: false
}
})

// Instrument for code coverage in development mode
const REACT_APP_ENV = process.env.REACT_APP_ENV ?? process.env.NODE_ENV ?? 'production'
Expand All @@ -97,7 +111,22 @@ function webpackOverride(config) {
return config
}

module.exports = {
const configOverride = {
webpack: webpackOverride,
jest: (config) => config
jest: (config) => {
/**
* @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',
'multiformats/bases/(.*)$': '<rootDir>/node_modules/multiformats/cjs/src/bases/$1.js'
Copy link
Contributor

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'

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 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.

Copy link
Member Author

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

}
Comment on lines +123 to +127
Copy link
Member Author

Choose a reason for hiding this comment

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

this should go away when we update to the latest.

})
}
}

export default configOverride
Loading