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

Do not write package.json when there are no changes (affects addon.appReexports() and addon.publicAssets()) #1423

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented May 12, 2023

Resolves #1420
but because app-re-exports isn't the only thing writing to package.json, the same conditional guard was also applied to public-assets.

How to test:

  • link the local package to a v2 addon
  • start rollup in watch mode
  • in a separate terminal, watch the modified date of the package.json via: watch -n 1 'stat -f "%Sc" package.json '
    • on macOS, you may need to brew install watch
  • make a change to a component within the v2 addon
  • note that the package.json isn't change, and the modified date is not changed either
  • add/remove a component or change the publicEntrypoints or change the appReexports in the rollup config.

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review May 12, 2023 16:27
@NullVoxPopuli NullVoxPopuli force-pushed the do-not-write-app-re-exports-when-not-needed branch from 71416c7 to 4bc6a64 Compare May 12, 2023 16:28
@NullVoxPopuli NullVoxPopuli changed the title rollup-app-reexports: do not write package.json when there are no changes Do not write package.json when there are no changes (affects addon.appReexports() and addon.publicAssets()) May 12, 2023
@NullVoxPopuli NullVoxPopuli force-pushed the do-not-write-app-re-exports-when-not-needed branch 3 times, most recently from 139e91f to c373485 Compare May 18, 2023 01:27
@NullVoxPopuli NullVoxPopuli force-pushed the do-not-write-app-re-exports-when-not-needed branch from c373485 to d3a8b31 Compare May 18, 2023 19:59
@NullVoxPopuli
Copy link
Collaborator Author

Working on a test atm -- holding off on merge.

@NullVoxPopuli NullVoxPopuli force-pushed the do-not-write-app-re-exports-when-not-needed branch 2 times, most recently from 31070b0 to f4a926f Compare May 18, 2023 23:25
@NullVoxPopuli NullVoxPopuli force-pushed the do-not-write-app-re-exports-when-not-needed branch from f4a926f to 1ef57d6 Compare May 18, 2023 23:27
addon.linkDevDependency('rollup', { baseDir: __dirname });
})
.forEachScenario(scenario => {
Qmodule(scenario.name, function (hooks) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there are 3 tests here

  • 2 watch tests
  • 1 re-build


stop = () => this.#singletonAbort?.abort();
settled = () => this.#waitForBuildPromise;
get lastBuild() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the only public api in this class that's not used atm.
I couldn't decide if it would be useful information to have for later.

//
// the `stat` is measured in `ms`, so it's still pretty fast
await aBit(10);
await fs.rm(path.join(addon.dir, 'src/components/demo.js'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am very weary of all the ways things can go bad with tests that do file system deletes
I believe for this test renaming a file would be sufficient and less risk prone especially on different OS's
also path.join takes multiple arguments and connects them with the system separator so specifying src comp... as individual parameters will make it a more robust implementation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

path.join takes multiple arguments and connects them with the system separator so specifying src comp..

path.join converts inline separators to the platform-specific separator, if needed.
info here: https://nodejs.org/api/path.html#pathjoinpaths
the key here is that after a path is joined, it's normalized, which does the platform-specific separator fixing.

all the ways things can go bad with tests that do file system deletes

what are those things?

Copy link
Collaborator

Choose a reason for hiding this comment

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

addon.dir suddenly having a glob like structure that would target more paths than expected.
which can happen outside of changes to this test if you consider something being updated to support/test v2 addons that need to span more folders than just one as it does today
the thought process for me is more on the lines of how to reduce the worst possible case rather than try to avoid something bad from happening

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

suddenly having a glob

This would cause a cascade of other problems, too: https://github.com/ef4/scenario-tester/blob/main/index.ts#L185

I've opened embroider-build/scenario-tester#9 to get clarification, and will PR over there depending on the answer.

@NullVoxPopuli NullVoxPopuli merged commit d950e6a into embroider-build:main May 30, 2023
@NullVoxPopuli NullVoxPopuli deleted the do-not-write-app-re-exports-when-not-needed branch May 30, 2023 16:01
@NullVoxPopuli NullVoxPopuli added the bug Something isn't working label May 30, 2023
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.

rollup-app-reexports writes package.json even if not needed
3 participants