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 embroider builds #432

Conversation

bendemboski
Copy link
Member

Embroider builds fail when mirage's files are not included in the build (e.g. production builds) because our mirage helper module's static import fails to resolve. So import dynamically inside the handler function when we know mirage's files must be present.

Copy link
Collaborator

@jelhan jelhan left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this. The solution looks good to me. But just to be safe I would prefer to land fixes to CI first.

A test scenario for Embroider would also be great. But not sure if adding one before upgrading dependencies would cause a lot additional work. If so, merging without would be fine for me.

@bendemboski
Copy link
Member Author

@jelhan I looked into enabling embroider test scenarios, and as you suspected they are difficult to set up with such old dependencies, so I gave up. Also, as I discovered over in my other PR upgrading dependencies, the embroider scenarios don't pick up this failure because this only fails when building under embroider and excluding the mirage files from the build. I briefly looked into adding an ember-try scenario that excludes embroider files, but that messed up the tests that ember-file-upload tests that do rely on mirage, and I couldn't easily figure out a way to disable them under that one scenario, so I gave up.

The good news is that I manually verified that this fixes the embroider-but-no-mirage-files build issue. I totally understand wanting to wait until CI is fixed, and I have a workaround for the issue in my app, so I wouldn't object either way.

FWIW, my workaround is adding this to my app's webpack config (although I don't use the mirage helpers at all, so I was able to just do this unconditionally which made it easier):

          module: {
            rules: [
              {
                include: [
                  /\/ember-file-upload\/mirage\//,
                ],
                use: 'null-loader',
              },
            ],
          },

@gilest
Copy link
Collaborator

gilest commented Jun 12, 2021

@bendemboski Please rebase 🙇

@gilest
Copy link
Collaborator

gilest commented Jul 5, 2021

@bendemboski could you please rebase and push so that the new CI can run 🙇

If you don't I'll open a new PR with your changes soon so that we can get ahead of the embroider release.

Embroider builds fail when mirage's files are not included in the build (e.g. production builds) because our mirage helper module's static import fails to resolve. So import dynamically inside the handler function when we know mirage's files must be present.
@bendemboski bendemboski force-pushed the fix-embroider-mirage branch from 57aefe2 to d93e652 Compare July 6, 2021 01:47
@bendemboski
Copy link
Member Author

@gilest I rebased, but the optimized scenario is failing, which I was worried might happen. I don't know if it's possible to get embroider to support this kind of dynamic importing from addon/ without it forcing ember-cli-mirages code to be included in all builds. If it is, I think it must require adding something to the package rules, and last I checked, there was no way for addons to do that directly.

I think the real solution is probably conditionally including this file in the build, as discussed here. I'm not sure if there's really a way to support optimized settings as long as addon/ code tries to import code that may or may not be present in the build.

@gilest
Copy link
Collaborator

gilest commented Sep 25, 2021

Thoughts on a way forward with this @bendemboski ? I see the issues and PRs in ec-mirage are still open 😅

@bendemboski
Copy link
Member Author

@gilest I'd have to pack this all back into my head to be certain, but I'm fairly sure that the best path forward would be to port this solution to ember-file-upload so we can conditionally include the mirage-related code only when we know mirage is included in the build. Then hopefully-someday when mirage exposes a public API for making that determination, we can switch to using that, which should have no impact on users or compatibility concerns.

@gilest
Copy link
Collaborator

gilest commented Oct 14, 2021

@bendemboski Thanks! With this guidance I've staged #567

@gilest gilest closed this Oct 14, 2021
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.

3 participants