acoustep / ember-cli-foundation-6-sass

MIT License
64 stars 33 forks source link

Adds Fastboot 1.0.0 support #77

Closed jurecuhalev closed 6 years ago

jurecuhalev commented 7 years ago

Moves tree processing from included() into treeForVendor so that we can use fastbootTransform on JS funnel (which guards for Fastboot).

All the JS processing logic now happens in treeForVendor because it builds a modified verision of JS files that are only later imported in included.

I've also replaced babel with broccoli-babel-transpiler so that it's part of treeForVendor funnel pipeline.

Looking at this code, it seems that supporting installations via npm shouldn't be too much extra work and it would allow the plugin to drop bower.json as dependancy. But that's for another PR and ticket.

Needs a review and more testing as right now I've tested it mainly with Foundation 6.3.x and ember-cli: 2.13.2 (and Ember 2.13.x).

It also extra note that it's incompatible with versions of Fastboot before 1.0.0.

Ref #74 #75

GCheung55 commented 7 years ago

Thanks for the PR! Any suggestions for testing? I think I need to spin up a test app in Fastboot.

How difficult would it be to add https://github.com/kaliber5/ember-fastboot-addon-tests ?

jurecuhalev commented 7 years ago

So actually, this PR will conflict with #78 . I suggest that one gets merged first then I'll update again against master to be Fastboot friendly.

Since Fastboot 1.0.0 support breaks older fastboot packages and change to npm usage, we'll probably need some upgrade notice in the README. So this will be a good time for a version bump and for upgrades to check both at the same time.

I can also take a look at adding Fastboot tests. Any thoughts on adding tests automatic test checks for PR's?

GCheung55 commented 7 years ago

@gandalfar I've just merged #78. As for adding tests, I think we need to setup Travis so it runs automatically when PR's are submitted. I've filed a ticket for Travis, #80.

seacar commented 7 years ago

What is the status of this? We really need this addon to work with fastboot!

Thanks!

GCheung55 commented 7 years ago

@gandalfar how are things looking for you? Can you resolve the conflicts and take a stab at writing the Fastboot tests?

I think Travis CI has been setup to run on PRs. Is that right, @acoustep?

acoustep commented 7 years ago

Yes, Travis CI has been set up for PRs.

jurecuhalev commented 7 years ago

I'll give it a look this week.

jurecuhalev commented 7 years ago

I've pushed 'merge' that uses new npm resolve pattern.

If somebody else can also test this branch, it would be great. Next step for me is to figure out how to write fastboot tests (and to fix ember-beta failing test)

jurecuhalev commented 7 years ago

It turns out that these tests are already failing in master and I currently have no idea how to fix it, so I've opened up an issue for it - #88. Any ideas?

jurecuhalev commented 7 years ago

I've merged current master changes and tests now pass.

GCheung55 commented 7 years ago

@gandalfar Do you happen to have a test project you're using to test these changes? I'd like to check it out. :)

GCheung55 commented 7 years ago

@gandalfar Sorry, I somehow committed/pushed changes to your branch. I don't even know how that is possible.

jurecuhalev commented 7 years ago

@GCheung55 no problem. I'll just leave the commit in, since I assume it will be part of master anyway.

For testing this out, I think the best way would be to ensure that dummy app in this addon provides foundation's kitchen sink CSS + some components and this would allow us to also easily ensure Fastboot compatibility.

jurecuhalev commented 7 years ago

I've added fastboot to dummy app and it's working with 'foundationJS: 'all'. It's not working with hand-picked JS modules (I've left them commented out in ember-cli-build.js) for easier testing. It looks like it's either not transpiled, or something else is wrong.

GCheung55 commented 7 years ago

@gandalfar I dug into the issue where foundationJS: [...] causes and issue.

Basically foundationJS: 'all' loads the foundation-sites/js/foundation-sites.js file, which is basically an UMD that adds Foundation to the window.

foundationJS: [] on the other hand loads each module, but there's nothing importing those modules to add Foundation to the window or load the plugins on to the Foundation object.

I think we need to create a file that loads those modules - probably dynamically using something like broccoli-create-file. This file could also export Foundation, and just have zf-widget import it.

@acoustep What are your thoughts here? Should we get that part addressed before merging this?

acoustep commented 6 years ago

That sounds fine to me (using broccoli-create-file). I'd say it would be best to address it before merging.

GCheung55 commented 6 years ago

@acoustep @gandalfar Sorry for the delay guys. I've finally made time to switch to using Rollup to load foundation-sites.

Please check things out and try out the fastboot-ness.

GCheung55 commented 6 years ago

While trying this out on my own project, I came across a breaking change that's been introduced.

import 'foundation-sites is now needed for those who are manually triggering this.$().foundation() in custom modules.

This basically removes the dependency on Foundation global.

GCheung55 commented 6 years ago

Happy New Year! @acoustep @gandalfar ping! When this get's merged, consider publishing a minor or major version.

jurecuhalev commented 6 years ago

I'm actually not sure what needs to be done in this PR to be ready for merge. @GCheung55 do you have open issues list?

GCheung55 commented 6 years ago

@gandalfar I don't have any open issues. I just pinged you to review what I've done and if things make sense. I did introduce a breaking change that requires import foundation-sites for those who depended on the Foundation global.

If things look good to you, I think all that we're waiting for is @acoustep. If he says it's good I can just merge and he'll need to publish to npm.

jurecuhalev commented 6 years ago

I was following your changes and they look good to me. I also think that going with explicit imports for globals is better and feels in line with what's happening in Ember ecosystem in general.

jurecuhalev commented 6 years ago

Hey @acoustep any chance to get this reviewed and merged? Thanks.

jurecuhalev commented 6 years ago

I've pushed a bump of deps that fixes Ember 2.18's depreciation:

DEPRECATION: An addon is trying to access project.nodeModulesPath. This is not a reliable way to discover npm modules. Instead, consider doing: require("resolve").sync(something, { basedir: project.root }). Accessed from: DependencyVersionChecker.NPMDependencyVersionChecker (/Users/gandalf/hacking/ember-cli-foundation-6-sass/node_modules/ember-cli-version-checker/src/npm-dependency-version-checker.js:11:32)

This PR might not be the best place for additional changes, but I'm not sure what to fork from, so I'm continuing development in it for now.

acoustep commented 6 years ago

Merged and publishing to npm shortly 🙂