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

fix: reset HTMLEntrypoint state after build (fix #1211) #1214

Closed
wants to merge 1 commit into from

Conversation

jacobq
Copy link

@jacobq jacobq commented Jun 3, 2022

I'm not sure if this is "the right solution", but it did seem the most obvious one to me as an outsider, and it does appear to work (fixes #1211). I don't love that it exposes a method (HTMLEntrypoint::initialize) that should probably be private somehow (e.g. calling it after construction but before being "done" may lead to weird/inconsistent states), but I needed a way for the Webpack plugin to reset the HTMLEntrypoint to its pristine/initial state so that it could be reused for subsequent calls to build that used the cached webpack instance (and perhaps a stern warning in a doc comment would be sufficient here...). Open to feedback / alternatives, of course, just wanted to help get this unblocked.

@krisselden I think you made the original PR that refactored the file output to a Webpack plugin. Do you have thoughts on this?

@jacobq
Copy link
Author

jacobq commented Jun 3, 2022

@ef4 Would you mind approving CI run to make sure I didn't accidentally break any tests? (My local testing only consisted of linking my bug repro app and confirming that it worked correctly with this change though I am running yarn test locally right now in the meantime.)

So far tests are passing though I see some 4.x deprecation warnings like this one:

remove-owner-inject
EPRECATION: As of Ember 4.0.0, owner.inject no longer injects values into resolved instances, and calling the method has been deprecated. Since this method no longer does anything, it is fully safe to remove this injection. As an alternative to this API, you can refactor to explicitly inject `_ajaxRequest` on `adapter`, or look it up directly using the `getOwner` API. [deprecation id: remove-owner-inject] This will be removed in Ember 5.0.0. See https://deprecations.emberjs.com/v4.x#toc_implicit-injections for more details.
        at logDeprecationStackTrace (/tmp/tmp-711579jC8mciX1YtxX/dist/assets/@ember/debug/lib/deprecate.js:112:1)
        at HANDLERS.<computed> (/tmp/tmp-711579jC8mciX1YtxX/dist/assets/@ember/debug/lib/handlers.js:26:1)
        at raiseOnDeprecation (/tmp/tmp-711579jC8mciX1YtxX/dist/assets/@ember/debug/lib/deprecate.js:141:1)
        at HANDLERS.<computed> (/tmp/tmp-711579jC8mciX1YtxX/dist/assets/@ember/debug/lib/handlers.js:26:1)
        at invoke (/tmp/tmp-711579jC8mciX1YtxX/dist/assets/@ember/debug/lib/handlers.js:38:1)
        at deprecate (/tmp/tmp-711579jC8mciX1YtxX/dist/assets/@ember/debug/lib/deprecate.js:187:1)
        at Registry.injection (/tmp/tmp-711579jC8mciX1YtxX/dist/assets/@ember/-internals/container/index.js:828:1)
        at App.inject (/tmp/tmp-711579jC8mciX1YtxX/dist/assets/@ember/-internals/runtime/lib/mixins/registry_proxy.js:223:1)
        at Object.initialize (webpack://app-template/./initializers/ajax.js?:41:17)
        at /tmp/tmp-711579jC8mciX1YtxX/dist/assets/@ember/engine/index.js:128:1
        at Vertices.each (/tmp/tmp-711579jC8mciX1YtxX/dist/assets/dag-map.js:231:1)
        at Vertices.walk (/tmp/tmp-711579jC8mciX1YtxX/dist/assets/dag-map.js:145:1)
        at DAG.each (/tmp/tmp-711579jC8mciX1YtxX/dist/assets/dag-map.js:75:1)
        at DAG.topsort (/tmp/tmp-711579jC8mciX1YtxX/dist/assets/dag-map.js:83:1)
        at App._runInitializer (/tmp/tmp-711579jC8mciX1YtxX/dist/assets/@ember/engine/index.js:155:1)
        at App.runInitializers (/tmp/tmp-711579jC8mciX1YtxX/dist/assets/@ember/engine/index.js:126:1)
        at App._bootSync (/tmp/tmp-711579jC8mciX1YtxX/dist/assets/@ember/application/lib/application.js:578:1)
        at App.boot (/tmp/tmp-711579jC8mciX1YtxX/dist/assets/@ember/application/lib/application.js:545:1)
        at EmberApp._visit (/home/user/tmp/embroider/node_modules/fastboot/src/ember-app.js:257:15)
        at processTicksAndRejections (internal/process/task_queues.js:95:5)
        at EmberApp.visit (/home/user/tmp/embroider/node_modules/fastboot/src/ember-app.js:325:7)
        at FastBoot.visit (/home/user/tmp/embroider/node_modules/fastboot/src/index.js:86:18)
        at Object.visit (/home/user/tmp/embroider/tests/scenarios/helpers.ts:39:16)
        at Object.<anonymous> (/home/user/tmp/embroider/tests/scenarios/fastboot-app-test.ts:110:20)

@ef4
Copy link
Contributor

ef4 commented Jun 5, 2022

Thanks for taking a look at this.

It's intentional that the HTMLEntrypoint preserves state across builds. The idea is that you don't bother re-parsing an HTML file that hasn't changed. It's only the contents of the placeholders that need to get recreated each build.

I think what's missing is a call to placeholder.clear() right about here.

@ef4
Copy link
Contributor

ef4 commented Jun 9, 2022

Superseded by #1219.

@ef4 ef4 closed this Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

live reload broken (duplicate scripts like vendor.js appear)
2 participants