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

Embroider #406

Merged
merged 2 commits into from Apr 20, 2021
Merged

Embroider #406

merged 2 commits into from Apr 20, 2021

Conversation

alias-mac
Copy link
Contributor

@alias-mac alias-mac commented Apr 16, 2021

Changes proposed in this pull request

Use @embroider/test-setup

Ember addons benefit on using this addon provided by embroider directly.
Read more information about it here.

Fix warnings provided by embroider build:

While running ember try:one embroider-safe or ember try:one embroider-optimized you will see warnings during the build:

    WARNING in /private/var/folders/1m/cqqx124s5gj0dn95jmbp91p0000mgw/T/embroider/a0a719/node_modules/ember-data/node_modules/ember-inflector/index.js 4:0-48
    "export 'defaultRules' was not found in './lib/system'
     @ ./assets/dummy.js
    
    WARNING in ./helpers/entries.js 1:0-60
    "export 'entries' was not found in '../../../helpers/entries'
     @ ./assets/dummy.js
    
    WARNING in ./helpers/from-entries.js 1:0-69
    "export 'fromEntries' was not found in '../../../helpers/from-entries'
     @ ./assets/dummy.js
    
    WARNING in ./helpers/pick.js 1:0-54
    "export 'pick' was not found in '../../../helpers/pick'
     @ ./assets/dummy.js
    
    WARNING in ./helpers/values.js 1:0-58
    "export 'values' was not found in '../../../helpers/values'
     @ ./assets/dummy.js

The ones related with this addon are fixed (in the second commit).

I had to rename keys to entries to match the expectation in app/ re-export, but this can be considered a breaking change, so perhaps it will be better to change the re-export to be keys instead of entries, although that breaks the convention from the other re-exports. Open to suggestions on how you want to proceed here.

@alias-mac
Copy link
Contributor Author

This is related with #403 but I took a different approach (since I'm not removing the re-exports).

@alias-mac alias-mac mentioned this pull request Apr 16, 2021
@snewcomer
Copy link
Contributor

@alias-mac Thanks for this PR! I'm fine either way. Would you mind merging in the main branch and see what happens to the test suite?

#404

@alias-mac
Copy link
Contributor Author

@snewcomer rebased and ran the tests locally and they pass, but they are failing in CI.

I was able to reproduce it when I run it with CI=true environment too locally. After a bit of experimentation I was able to confirm that it is due to the fact that we test against IE11 in CI. When I remove that it passes.

Based on embroider's changelog it seems that IE11 is supported. I'll try to take a stab at it and see what might be needed to get this fixed, but open to ideas.

Ember addons benefit on using this addon provided by embroider directly.
Read more information about it here:
https://github.com/embroider-build/embroider/tree/master/packages/test-setup
The helpers re-exported in `app/` are expecting these to be defined.
@alias-mac
Copy link
Contributor Author

@snewcomer I synced with @rwjblue and he pointed me in the right direction on what was needed to make this pass.
The addon blueprint will contain new options by default, so I just added them here.
The test-setup function should probably support that by default, but for now this solves the problem.

Let me know what are the next steps to get this in. Thanks!

Copy link
Contributor

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

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

Woop woop!

@snewcomer snewcomer merged commit ba9d6e3 into DockYard:master Apr 20, 2021
@alias-mac alias-mac deleted the embroider branch April 20, 2021 18:56
@rwjblue
Copy link
Contributor

rwjblue commented Jun 7, 2021

@snewcomer any chance for a release with this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants