-
Notifications
You must be signed in to change notification settings - Fork 54
fix(metro): track imported CSS dependencies for hot reload #676
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| const assert = require('node:assert/strict') | ||
| const { execFileSync } = require('node:child_process') | ||
| const fs = require('node:fs') | ||
| const path = require('node:path') | ||
| const os = require('node:os') | ||
| const { createRequire } = require('node:module') | ||
|
|
||
| // Run directly: node packages/uniwind/tests/native/bundler/fixtures/metro-css-hmr.cjs bare ios | ||
| const packageRoot = path.resolve(__dirname, '../../../..') | ||
| const repo = path.resolve(packageRoot, '../..') | ||
| const req = createRequire(path.join(repo, 'package.json')) | ||
| const { default: IncrementalBundler } = req('metro/private/IncrementalBundler') | ||
| const [kind = 'bare', platform = 'ios'] = process.argv.slice(2) | ||
| const root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'uniwind-live-metro-'))) | ||
| const entry = path.join(root, 'index.js') | ||
| const css = path.join(root, 'global.css') | ||
| const tokens = path.join(root, 'tokens.css') | ||
| const nested = path.join(root, 'nested.css') | ||
| const runtime = path.join(root, 'runtime.js') | ||
| const results = { kind, platform, pid: process.pid, updates: [] } | ||
|
|
||
| async function main() { | ||
| fs.symlinkSync(path.join(repo, 'node_modules'), path.join(root, 'node_modules'), 'dir') | ||
| const isolatedPackage = path.join(root, 'uniwind') | ||
| // Build current sources in isolation so the test cannot use stale dist output. | ||
| execFileSync(process.execPath, [ | ||
| '--input-type=module', | ||
| '-e', | ||
| ` | ||
| import { build } from 'unbuild'; | ||
| await build(process.cwd(), false, { | ||
| hooks: { | ||
| 'build:prepare': ctx => { | ||
| ctx.options.entries = ctx.options.entries.filter(entry => entry.name === 'metro/index' || entry.name === 'metro/transformer'); | ||
| ctx.options.outDir = process.argv[1]; | ||
| // This fixture does not build the other package exports. | ||
| ctx.options.failOnWarn = false; | ||
| }, | ||
| }, | ||
| }); | ||
| `, | ||
| path.join(isolatedPackage, 'dist'), | ||
| ], { cwd: packageRoot, stdio: 'pipe', timeout: 15_000 }) | ||
| const { withUniwindConfig } = require(path.join(isolatedPackage, 'dist/metro/index.cjs')) | ||
| fs.writeFileSync(path.join(root, 'package.json'), JSON.stringify({ name: 'uniwind-hmr-fixture', private: true })) | ||
| fs.writeFileSync( | ||
| path.join(root, 'babel.config.js'), | ||
| `module.exports = { presets: [${JSON.stringify(req.resolve(kind === 'expo' ? 'babel-preset-expo' : '@react-native/babel-preset'))}] };`, | ||
| ) | ||
| fs.writeFileSync(entry, `require('./global.css'); global.__fixtureSession = 'retained';`) | ||
| fs.writeFileSync(runtime, 'module.exports = { Uniwind: { __reinit() {} } };') | ||
| fs.writeFileSync(css, '@import "./tokens.css"; @tailwind utilities; @source inline("bg-brand");') | ||
| fs.writeFileSync(tokens, '@import "./nested.css";') | ||
| fs.writeFileSync(nested, '@theme static { --color-brand: #123456; }') | ||
| const { getDefaultConfig } = req(kind === 'expo' ? '@expo/metro-config' : '@react-native/metro-config') | ||
| let config = await getDefaultConfig(root) | ||
| config.maxWorkers = 1 | ||
| config.watchFolders = [root, path.join(repo, 'node_modules'), path.join(repo, 'packages/uniwind')] | ||
| config.resolver.useWatchman = false | ||
| config.resolver.nodeModulesPaths = [path.join(repo, 'node_modules')] | ||
| config.resolver.resolveRequest = (context, moduleName, target) => | ||
| moduleName === 'uniwind' | ||
| ? { type: 'sourceFile', filePath: runtime } | ||
| : context.resolveRequest(context, moduleName, target) | ||
| config.reporter = { update() {} } | ||
| config = withUniwindConfig(config, { | ||
| cssEntryFile: path.relative(process.cwd(), css), | ||
| dtsFile: path.join(root, 'uniwind-types.d.ts'), | ||
| }) | ||
| results.expoWorker = config.transformer.uniwind.isExpoProject | ||
| assert.equal(results.expoWorker, kind === 'expo') | ||
| const bundler = new IncrementalBundler(config, { watch: true }) | ||
| let unlisten | ||
| try { | ||
| await bundler.ready() | ||
| let { revision } = await bundler.initializeGraph(entry, { | ||
| dev: true, | ||
| hot: true, | ||
| minify: false, | ||
| platform, | ||
| type: 'module', | ||
| unstable_transformProfile: 'default', | ||
| }, { customResolverOptions: {} }) | ||
| assert(revision.graph.dependencies.has(tokens), 'direct CSS missing from Metro graph') | ||
| assert(revision.graph.dependencies.has(nested), 'nested CSS missing from Metro graph') | ||
| const codeFor = file => revision.graph.dependencies.get(file).output.find(output => output.type.startsWith('js/')).data.code | ||
| assert(codeFor(css).includes('#123456'), 'initial token missing') | ||
| results.cssModules = [css, tokens, nested].map(file => path.basename(file)) | ||
| for (const color of ['#654321', '#123456']) { | ||
| const change = new Promise((resolve, reject) => { | ||
| const timeout = setTimeout(() => reject(new Error('Metro did not report a token-only change in 15s')), 15000) | ||
| const watcher = bundler.getBundler().getWatcher() | ||
| const listener = event => { | ||
| if (Array.from(event.changes.modifiedFiles, ([file]) => file).some(file => path.join(event.rootDir, file) === nested)) { | ||
| clearTimeout(timeout) | ||
| resolve() | ||
| } | ||
| } | ||
| watcher.on('change', listener) | ||
| unlisten = () => { | ||
| clearTimeout(timeout) | ||
| watcher.off('change', listener) | ||
| } | ||
| }) | ||
| fs.writeFileSync(nested, `@theme static { --color-brand: ${color}; }`) | ||
| await change | ||
| unlisten() | ||
| const update = await bundler.updateGraph(revision, false) | ||
| revision = update.revision | ||
| assert(update.delta.modified.has(css), 'CSS entry did not appear in the HMR delta') | ||
| assert(!update.delta.modified.has(entry), 'JavaScript entry changed during a token-only update') | ||
| assert(codeFor(css).includes(color), `Updated token ${color} missing from CSS entry`) | ||
| results.updates.push({ color, modified: [...update.delta.modified.keys()].map(file => path.basename(file)), reset: update.delta.reset }) | ||
| assert.equal(update.delta.reset, false) | ||
| } | ||
| console.log(JSON.stringify(results, null, 2)) | ||
| } finally { | ||
| unlisten?.() | ||
| await bundler.end() | ||
| } | ||
| } | ||
|
|
||
| main().catch(error => { | ||
| console.error(error) | ||
| process.exitCode = 1 | ||
| }).finally(() => fs.rmSync(root, { recursive: true, force: true })) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { execFileSync } from 'node:child_process' | ||
| import path from 'node:path' | ||
|
|
||
| // The child process uses real Metro workers and isolates their watchers from Jest. | ||
| test.each([ | ||
| ['bare', 'ios'], | ||
| ['bare', 'android'], | ||
| ['expo', 'ios'], | ||
| ['expo', 'android'], | ||
| ])('delivers token-only HMR updates with %s Metro on %s', (kind, platform) => { | ||
| const output = execFileSync(process.execPath, [path.join(__dirname, 'fixtures/metro-css-hmr.cjs'), kind, platform], { | ||
| encoding: 'utf8', | ||
| timeout: 30_000, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win Increase the test deadlines. The build and two sequential watcher waits can consume nearly 45 seconds. The 30-second child-process timeout and 35-second test-framework timeout can fail a valid slow run. Increase both deadlines: Proposed fix- timeout: 30_000,
+ timeout: 50_000,
@@
-}, 35_000)
+}, 55_000)🧰 Tools🪛 ast-grep (0.45.3)[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec. (detect-child-process-typescript) 🤖 Prompt for AI Agents |
||
| env: { ...process.env, NODE_ENV: 'development' }, | ||
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| }) | ||
| const result = JSON.parse(output) | ||
|
|
||
| expect(result.expoWorker).toBe(kind === 'expo') | ||
| expect(result.updates).toEqual([ | ||
| { color: '#654321', modified: expect.arrayContaining(['nested.css', 'global.css']), reset: false }, | ||
| { color: '#123456', modified: expect.arrayContaining(['nested.css', 'global.css']), reset: false }, | ||
| ]) | ||
| }, 35_000) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,164 @@ | ||
| import { transform as workerTransform } from 'metro-transform-worker' | ||
| import { mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' | ||
| import { tmpdir } from 'node:os' | ||
| import path from 'node:path' | ||
| import { transform } from '../../../src/bundler/adapters/metro/transformer' | ||
| import { UniwindBundlerConfig } from '../../../src/bundler/config' | ||
|
|
||
| jest.mock('metro-transform-worker', () => ({ transform: jest.fn() })) | ||
| jest.mock('@expo/metro-config', () => ({ unstable_transformerPath: 'metro-transform-worker' })) | ||
|
|
||
| const mockWorker = jest.mocked(workerTransform) | ||
| const options = { | ||
| dev: true, | ||
| minify: false, | ||
| platform: 'ios', | ||
| type: 'module', | ||
| inlineRequires: false, | ||
| experimentalImportSupport: false, | ||
| inlinePlatform: true, | ||
| unstable_transformProfile: 'default', | ||
| } satisfies Parameters<typeof transform>[4] | ||
|
|
||
| let root: string | ||
| let entry: string | ||
|
|
||
| beforeEach(() => { | ||
| root = realpathSync(mkdtempSync(path.join(tmpdir(), 'uniwind-css-imports-'))) | ||
| entry = path.join(root, 'app', 'global.css') | ||
| mkdirSync(path.dirname(entry), { recursive: true }) | ||
| writeFileSync(path.join(root, 'app', 'tokens.css'), '@theme static { --color-brand: #123456; }') | ||
| writeFileSync(entry, '@import "./tokens.css"; @tailwind utilities; @source inline("bg-brand");') | ||
| // Artifact generation is independent of the compiler/Metro dependency contract under test. | ||
| jest.spyOn(UniwindBundlerConfig.prototype, 'generateArtifacts').mockResolvedValue(undefined) | ||
| mockWorker.mockImplementation(async (_config, _root, _file, data) => ({ | ||
| dependencies: [], | ||
| output: [{ type: 'js/module', data: { code: data.toString(), lineCount: 1, map: [], functionMap: null } }], | ||
| })) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| jest.restoreAllMocks() | ||
| mockWorker.mockClear() | ||
| rmSync(root, { force: true, recursive: true }) | ||
| }) | ||
|
|
||
| const transformFile = async (file = entry, platform = 'ios', isExpoProject = false, type: 'module' | 'asset' = 'module') => { | ||
| const config = { | ||
| uniwind: { cssEntryFile: path.relative(process.cwd(), entry), isExpoProject }, | ||
| } as Parameters<typeof transform>[0] | ||
| const result = await transform(config, root, path.relative(root, file), readFileSync(file), { ...options, platform, type }) | ||
|
|
||
| return result.output[0].data.code as string | ||
| } | ||
|
|
||
| const runNativeModule = (code: string) => { | ||
| const reinit = jest.fn() | ||
| const required: Array<string> = [] | ||
| const requireModule = (name: string) => { | ||
| required.push(name) | ||
| return { Uniwind: { __reinit: reinit } } | ||
| } | ||
|
|
||
| new Function('require', code)(requireModule) | ||
| const [stylesheet, , fingerprint] = reinit.mock.calls[0] | ||
|
|
||
| return { required, fingerprint, variables: stylesheet({}).vars } | ||
| } | ||
|
|
||
| test.each(['ios', 'android'])('registers direct, nested and shared CSS imports once on %s', async platform => { | ||
| mkdirSync(path.join(root, 'shared')) | ||
| mkdirSync(path.join(root, 'app', 'node_modules', '@fixture'), { recursive: true }) | ||
| writeFileSync(path.join(root, 'shared', 'theme.css'), '@theme static { --spacing-shared: 8px; }') | ||
| symlinkSync(path.join(root, 'shared'), path.join(root, 'app', 'node_modules', '@fixture', 'theme'), 'dir') | ||
| writeFileSync(path.join(root, 'app', 'nested.css'), '@theme static { --spacing-nested: 4px; }') | ||
| writeFileSync(path.join(root, 'app', 'tokens.css'), '@import "./nested.css"; @theme static { --color-brand: #123456; }') | ||
| writeFileSync( | ||
| entry, | ||
| [ | ||
| '@import "./tokens.css";', | ||
| '@import "./nested.css";', | ||
| '@import "@fixture/theme/theme.css";', | ||
| '@tailwind utilities;', | ||
| '@source inline("bg-brand");', | ||
| ].join('\n'), | ||
| ) | ||
|
|
||
| const { required } = runNativeModule(await transformFile(entry, platform)) | ||
|
|
||
| expect(required.sort()).toEqual(['../shared/theme.css', './nested.css', './tokens.css', 'uniwind']) | ||
| }) | ||
|
|
||
| test('does not register CSS from installed dependencies', async () => { | ||
| mkdirSync(path.join(root, 'app', 'node_modules', 'vendor'), { recursive: true }) | ||
| writeFileSync(path.join(root, 'app', 'node_modules', 'vendor', 'theme.css'), '@theme static { --spacing-vendor: 2px; }') | ||
| writeFileSync(entry, '@import "./tokens.css"; @import "vendor/theme.css";') | ||
|
|
||
| expect(runNativeModule(await transformFile()).required).toEqual(['./tokens.css', 'uniwind']) | ||
| }) | ||
|
|
||
| test('recompiles token-only edits and updates the stylesheet fingerprint', async () => { | ||
| const before = runNativeModule(await transformFile()) | ||
| writeFileSync(path.join(root, 'app', 'tokens.css'), '@theme static { --color-brand: #654321; }') | ||
| const after = runNativeModule(await transformFile()) | ||
|
|
||
| expect(before.required).toContain('./tokens.css') | ||
| expect(before.variables['--color-brand']({})).toBe('#123456') | ||
| expect(after.variables['--color-brand']({})).toBe('#654321') | ||
| expect(after.fingerprint).not.toBe(before.fingerprint) | ||
| }) | ||
|
|
||
| test('updates import dependencies without changing an identical stylesheet fingerprint', async () => { | ||
| const before = runNativeModule(await transformFile()) | ||
| writeFileSync(path.join(root, 'app', 'extra.css'), '/* no styles */') | ||
| writeFileSync(entry, `${readFileSync(entry, 'utf8')}\n@import "./extra.css";`) | ||
| const added = runNativeModule(await transformFile()) | ||
| writeFileSync(entry, '@import "./tokens.css"; @tailwind utilities; @source inline("bg-brand");') | ||
| const removed = runNativeModule(await transformFile()) | ||
|
|
||
| expect(added.required).toEqual(['./extra.css', './tokens.css', 'uniwind']) | ||
| expect(removed.required).toEqual(before.required) | ||
| expect(added.fingerprint).toBe(before.fingerprint) | ||
| expect(removed.fingerprint).toBe(before.fingerprint) | ||
| }) | ||
|
|
||
| test.each(['ios', 'android'])('transforms non-entry CSS into an empty JS module in plain Metro on %s', async platform => { | ||
| const file = path.join(root, 'app', 'tokens.css') | ||
|
|
||
| expect(await transformFile(file, platform)).toBe('') | ||
| expect(mockWorker.mock.calls[0][2]).toBe(path.join('app', 'tokens.css.js')) | ||
| }) | ||
|
|
||
| test('leaves non-entry CSS handling to Expo', async () => { | ||
| const file = path.join(root, 'app', 'tokens.css') | ||
|
|
||
| expect(await transformFile(file, 'ios', true)).toBe(readFileSync(file, 'utf8')) | ||
| expect(mockWorker.mock.calls[0][2]).toBe(path.join('app', 'tokens.css')) | ||
| expect(runNativeModule(await transformFile(entry, 'ios', true)).required).toContain('./tokens.css') | ||
| }) | ||
|
|
||
| test('preserves web CSS output without native dependency requires', async () => { | ||
| const code = await transformFile(entry, 'web') | ||
|
|
||
| expect(code).toContain('--color-brand: #123456') | ||
| expect(code).not.toContain('require(') | ||
| expect(mockWorker.mock.calls[0][2]).toBe(path.join('app', 'global.css')) | ||
| const file = path.join(root, 'app', 'tokens.css') | ||
| expect(await transformFile(file, 'web')).toBe(readFileSync(file, 'utf8')) | ||
| }) | ||
|
|
||
| test('preserves asset and JavaScript transformations', async () => { | ||
| const file = path.join(root, 'app', 'tokens.css') | ||
| expect(await transformFile(file, 'ios', false, 'asset')).toBe(readFileSync(file, 'utf8')) | ||
| const script = path.join(root, 'app', 'index.js') | ||
| writeFileSync(script, 'module.exports = 123') | ||
| expect(await transformFile(script)).toBe('module.exports = 123') | ||
| }) | ||
|
|
||
| test('keeps hidden directories and spaces in local CSS paths relative', async () => { | ||
| mkdirSync(path.join(root, 'app', '.theme')) | ||
| writeFileSync(path.join(root, 'app', '.theme', 'brand tokens.css'), '@theme static { --color-brand: #123456; }') | ||
| writeFileSync(entry, '@import "./.theme/brand tokens.css";') | ||
|
|
||
| expect(runNativeModule(await transformFile()).required).toEqual(['./.theme/brand tokens.css', 'uniwind']) | ||
| }) |
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.
We shouldn't really have those for production build, only for development