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

Watch previous detected files in publicEntrypoints plugin #2208

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

patricklx
Copy link
Contributor

@patricklx patricklx commented Dec 11, 2024

  1. bug
    in public entrypoints we only add watch files. not dirs. When a file is removed, rollup also stops watching that file and ignores it if its re-created.
    We need to watch previous detected files.

  2. Testing Bug
    Package.json is updated during the process which triggers a new build, we need to wait

Other things:
. Need to call result.close https://rollupjs.org/javascript-api/#rollup-watch
Not sure about nodejs/node#4760

@patricklx patricklx force-pushed the fix-v2-addon-dev-watch branch 2 times, most recently from 23bf8f8 to 3652b36 Compare December 12, 2024 13:18
@patricklx patricklx changed the title fix windows v2 addon watch test fix v2 addon watch test Dec 12, 2024
@ef4
Copy link
Contributor

ef4 commented Dec 13, 2024

The fix in public-entry points looks good.

The changes to the test waiting don't make sense to me. We're doubling down on time-based testing here when we should have none. If every single nextBuild() needs to be sandwiched between delays, then nextBuild is simply wrong.

The point of nextBuild is to observe rollup completing a full build. A delay before calling nextBuild makes no sense, because it's not supposed to matter. You can call it as early as you like and it will still wait until rollup decides to build again.

A delay after nextBuild would only be needed after rollup reports a build complete, we still can't observe its output on the filesystem. If we're sure that's happening, then we should build polling into nextBuild to account for that. But I am skeptical that is actually happening.

@ef4
Copy link
Contributor

ef4 commented Dec 13, 2024

If anything, the possible race condition I see here would be calling nextBuild too late, not too early. Like, if you do fs.rm() and then your own process ends up slow enough that rollup finishes its build before you can initiate nextBuild, then your test might hang because you missed the build. But I also doubt this is happening, as the time scales involved in a direct filesystem mutation vs a whole rollup build are so different.

@patricklx
Copy link
Contributor Author

patricklx commented Dec 13, 2024

the time delays are more for preventing chokidar from throttling and merging events that it thinks are atomic. but maybe those are not needed anymore

@patricklx
Copy link
Contributor Author

patricklx commented Dec 13, 2024

If anything, the possible race condition I see here would be calling nextBuild too late, not too early. Like, if you do fs.rm() and then your own process ends up slow enough that rollup finishes its build before you can initiate nextBuild, then your test might hang because you missed the build. But I also doubt this is happening, as the time scales involved in a direct filesystem mutation vs a whole rollup build are so different.

okay, so that could then be related to the 150ms delay I introduced. But there was a delay before nextBuild already. so it could happen, but probably much less probability with 10ms.
with defer i mean the defer call on START event

@patricklx
Copy link
Contributor Author

i get failures when i try again with 10ms

@patricklx
Copy link
Contributor Author

logging start / end events it shows that we have 2 builds when we expect one. I suspect the update to package.json is triggering it.

> @embroider/[email protected] test D:\a\embroider\embroider\tests\scenarios
> qunit --require ts-node/register *-test.ts "--filter" "/v2-addon-dev-watch/"

TAP version 13
Test "deleting a component from src should delete it from dist" took longer than 3000ms, but no timeout was set. Set QUnit.config.testTimeout or call assert.timeout() to avoid a timeout in QUnit 3. https://qunitjs.com/api/config/testTimeout/
start
end
start
end
start
ok 1 v2-addon-dev-watch > Watching the addon via rollup -c -w > files are correctly synced > deleting a component from src should delete it from dist
end
not ok 2 v2-addon-dev-watch > Watching the addon via rollup -c -w > files are correctly synced > create a component in src should create it in dist
  ---
  message: "Expected D:\\temp\\tmp-5568BF4wsMXoBI26\\dist\\_app_\\components\\other.js to exist"
  severity: failed
  actual  : false
  expected: true
  stack: |
        at Object.<anonymous> (D:\a\embroider\embroider\tests\scenarios\v2-addon-dev-watch-test.ts:199:20)
  ...
start
end
ok 3 v2-addon-dev-watch > Watching the addon via rollup -c -w > files are correctly synced > updating hbs modifies generated colocated js
start
end
not ok 4 v2-addon-dev-watch > Watching the addon via rollup -c -w > files are correctly synced > deleting hbs file updates dist component file
  ---
  message: "Promise rejected during \"deleting hbs file updates dist component file\": Could not load D:\\temp\\tmp-5568BF4wsMXoBI26\\src\\components\\demo.hbs (imported by src/components/demo.js): ENOENT: no such file or directory, open 'D:\\temp\\tmp-5568BF4wsMXoBI26\\src\\components\\demo.hbs'"
  severity: failed
  stack: |
    Error: Could not load D:\temp\tmp-5568BF4wsMXoBI26\src\components\demo.hbs (imported by src/components/demo.js): ENOENT: no such file or directory, open 'D:\temp\tmp-5568BF4wsMXoBI26\src\components\demo.hbs'
        at async open (node:internal/fs/promises:639:25)
        at async Object.readFile (node:internal/fs/promises:1242:14)
        at async D:\a\embroider\embroider\node_modules\.pnpm\[email protected]\node_modules\rollup\dist\shared\rollup.js:25766:128
        at async Queue.work (D:\a\embroider\embroider\node_modules\.pnpm\[email protected]\node_modules\rollup\dist\shared\rollup.js:26145:32)
  ...
start
end
start
end
ok 5 v2-addon-dev-watch > Watching the addon via rollup -c -w > files are correctly synced > updating hbs content should not update unrelated files
start
end
ok 6 v2-addon-dev-watch > Watching the addon via rollup -c -w > files are correctly synced > updating hbs content should not update resulting app re-exported component
start
end
ok 7 v2-addon-dev-watch > Watching the addon via rollup -c -w > files are correctly synced > updating template only should update the dist output
start
end
ok 8 v2-addon-dev-watch > Watching the addon via rollup -c -w > files are correctly synced > deleting demo.js should make demo a template only component
start
end
ok 9 v2-addon-dev-watch > Watching the addon via rollup -c -w > files are correctly synced > creating demo.js should make demo a template colocated component
start
end
start
end
ok 10 v2-addon-dev-watch > Watching the addon via rollup -c -w > the package.json is not updated since it would be the same
start
end
start
end
start
end
ok 11 v2-addon-dev-watch > Watching the addon via rollup -c -w > the package.json *is* updated, since app-js changed
ok 12 v2-addon-dev-watch > the package.json *is* updated on a rebuild, since public assets changed

@patricklx
Copy link
Contributor Author

@ef4 this is ready for another look. I updated the description with my new findings. I think i have figured out the real issues.
I leave the debug logs in so that you can see them in ci.

@patricklx patricklx force-pushed the fix-v2-addon-dev-watch branch 6 times, most recently from b65cea8 to 1f2f893 Compare December 17, 2024 08:21
@patricklx
Copy link
Contributor Author

@patricklx patricklx force-pushed the fix-v2-addon-dev-watch branch 2 times, most recently from a0712c4 to 4a19c87 Compare December 17, 2024 09:12
@patricklx patricklx force-pushed the fix-v2-addon-dev-watch branch from 4a19c87 to d0f9bc0 Compare December 17, 2024 09:15
@ef4 ef4 merged commit 4070ba7 into embroider-build:main Dec 17, 2024
211 checks passed
@ef4 ef4 added internal bug Something isn't working and removed internal labels Dec 17, 2024
@ef4 ef4 changed the title fix v2 addon watch test Watch previous detected files in publicEntrypoints plugin Dec 17, 2024
@ef4 ef4 added bug Something isn't working and removed bug Something isn't working labels Dec 17, 2024
@patricklx patricklx deleted the fix-v2-addon-dev-watch branch December 17, 2024 16:57
@github-actions github-actions bot mentioned this pull request Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants