Add IcedCoffeeScript support, add lazy loading for CoffeeScript and Iced... #2599

internalfx · 2015-01-30T02:44:14Z

...CoffeeScript

Here is a possibility for adding lazy loading for CoffeeScript and IcedCoffeeScript.

Not super happy about using synchronous APIs, but it looked like I would have to tear the whole thing apart to do it Async.

Would love feedback, thanks.

…cedCoffeeScript

loicsaintroch · 2015-01-30T17:21:57Z

I like it. I am down with that only if @mikermcneil and @sgress454 agree too.

sgress454 · 2015-01-30T17:44:58Z

@internalfx Definitely appreciate the work you put in here. Merging this PR would be portentous of a future where moduleloader was 100,000 lines of code supporting every possible variation of Javascript. What I would really love to see instead is a PR that rips all of the |coffee|litcoffee|iced|liticed lines out, along with the overhead of enabling CoffeeScript and its variants, and makes a modular system where you just npm install sails-hook-icedcoffee if you that's what you want to write Sails apps in. It's not a trivial thing by any means, but I hope you can see how the course moduleloader is currently on is not sustainable.

internalfx · 2015-01-30T18:36:36Z

I agree that its not sustainable forever, but it's probably sustainable for a couple years. I'm not too worried about there being 5 new JavaScript variants within a few years, but who knows?

But long term, you are absolutely right. It's just gonna bloat into a bigger and bigger mess.

npm install sails-hook-icedcoffee would be really sweet. So clean, keeps core small. I like it.

I'm not entirely sure where to start on that. I'm gonna need guidance from the core team. I'm willing to look into doing that though. The Sails project has helped me, I'm happy to give back.

I do however think there is some value here in the interim though. Not even just for IcedCoffee. This allows the logger to log an error, and not just verbose. It also will not error at all unless they try to use Coffee or IcedCoffee.

Thoughts?

Oh and 5 points to @sgress454 for use of the word "portentous" 😄

mikermcneil · 2015-01-30T23:14:09Z

@internalfx would you mind implementing this as a hook config? i.e. replace the regexp def string with sails.config.moduleloader.filter here https://github.com/balderdashy/sails/blob/master/lib/hooks/moduleloader/index.js#L206

That way we can nip this one in the bud once and for all

internalfx · 2015-01-31T00:46:42Z

@mikermcneil Happy to do that, but I'm a little lost....

sails.config.moduleloader.filter doesn't appear to be defined.
sails.config.moduleloader isn't even defined.

where do you want it? I haven't looked under the hood a whole lot.

Also, the regex isn't the same everywhere, I'm assuming you want a global config? To be DRY'er?

Compare these two lines...
https://github.com/balderdashy/sails/blob/master/lib/hooks/moduleloader/index.js#L206
https://github.com/balderdashy/sails/blob/master/lib/hooks/moduleloader/index.js#L243

Some have |json| in there....

How would you like me to proceed?

internalfx · 2015-01-31T22:06:54Z

OK @mikermcneil . I had time to rethink my situation....

I was thinking about my response, and realized I should have put more effort into this. Sorry about that. Moving on...

So I "looked under the hood", and it seemed like the way you guys build sails.config is actually really simple. Pretty much wherever and whenever it suits you. Cool, I can do that.

regex isn't the same everywhere

Right, so fix it. (Was the response I imagined you saying in my head)
So I implemented an array of extensions to pull in, and I build those into regexes where necessary.

OK, how's this look now?

internalfx · 2015-02-05T22:27:10Z

@loicsaintroch @sgress454 @mikermcneil

...Bumpity Bump....busy week?

kevinburkeshyp · 2015-02-05T22:38:59Z

lib/hooks/moduleloader/index.js

+            require(path.join(localConfig.appPath, 'node_modules/coffee-script/register'));
+          }
+          catch (e1) {
+            sails.log.error('Please run `npm install coffee-script` to use CoffeScript!');


s/coffescript/coffeescript

internalfx · 2015-02-19T20:05:28Z

progress?

thoughts?

anything?

internalfx · 2015-04-11T20:08:40Z

Ok, I have had my rant on #2662.

Shall I update this to add livescript support? so that a merge will please everyone.

Pleasing everyone was my original intent with this.

…y_coffee Conflicts: lib/hooks/moduleloader/index.js

internalfx · 2015-04-11T20:29:09Z

lib/hooks/moduleloader/index.js

@@ -109,6 +80,71 @@ module.exports = function(sails) {
          layout: path.resolve(config.appPath, 'views/layout.ejs'),
        }
      };
+
+      var hasCoffee = false, hasIced = false, hasLive = false;
+


First we find out what languages are being used by walking the source files, and flagging what we find

tjwebb · 2015-04-11T20:30:53Z

@internalfx send me your email address when you get a chance re #2662 : tjwebb@balderdash.co

internalfx · 2015-04-11T20:36:45Z

done, I added some line notes.

This should be ready for review

tjwebb · 2015-04-12T18:25:40Z

Hey I just went through this PR, everything looks good.

@internalfx would you mind adding some documentation that explains the additional features that this provides, and some info on how to use it (or, if it's magic, then say so :) ).

@mikermcneil @sgress454 what are your thoughts? Things are always nicer as hooks, but I submit to you that existence is the more important feature in this case. Lots of stuff needs to be factored out into hooks anyway, and I think that will be a project in itself.

…y_coffee

internalfx · 2015-04-17T20:40:31Z

@tjwebb I'm working on cleaning this up some more....

tjwebb · 2015-04-17T20:41:12Z

Ok thanks @internalfx. Ping me when it's ready!

internalfx · 2015-04-17T20:41:20Z

Dont merge this yet...

internalfx · 2015-04-17T20:41:30Z

oh wow...you reply fast

internalfx · 2015-04-18T19:43:25Z

@tjwebb This is ready.

mikermcneil · 2015-04-22T18:40:54Z

@internalfx Re: synchronous vs asynchronous loading-- I'm right there with you. This is why we pulled out sails-build-dictionary back in 0.9-- to make the loading potentially asynchronous. But yeah, the standard answer from isaacs is that require is designed for synchronous use, and that it's ok because it only happens once. It's really only a minor developer UX problem, but one of these days, I'd like to help on that front in node/npm, but for now we'll have to live with it.

Anyways, thanks!

Add IcedCoffeeScript support, add lazy loading for CoffeeScript and Iced...

internalfx · 2015-04-22T23:37:22Z

@mikermcneil Yay! My projects can finally all run master again.

I wonder why require doesn't just take an optional callback? Such a system would keep all backwards compatibility.

On another topic, what are your thoughts on the idea of core hooks being override-able? (is that a word?)

Not sure how feasible that is, but then I could experiment with new core hooks. The moduleloader hook was so tied in that I couldn't make a nice clean way to add support for more languages. I would like to experiment with a custom moduleloader. (hope that doesn't make you cringe)

refs #3017, #2599

refs balderdashy#3017, balderdashy#2599

mikermcneil · 2016-09-16T19:47:22Z

For an update on this, see #3142 (comment) (let's funnel future discussion there as well so it's all in one place)

Add IcedCoffeeScript support, add lazy loading for CoffeeScript and I…

23db0c8

…cedCoffeeScript

Add sails.config.moduleloader, and build regexes from global config

508d6c1

kevinburkeshyp reviewed Feb 5, 2015
View reviewed changes

internalfx added 2 commits February 6, 2015 15:23

Merge remote-tracking branch 'upstream/master' into icy_coffee

6c309ac

fix typo

1f93490

internalfx added 3 commits February 19, 2015 13:12

Merge remote-tracking branch 'upstream/master' into icy_coffee

e768c64

Merge remote-tracking branch 'upstream/master' into icy_coffee

c40d9fa

Merge branch 'master' of github.com:balderdashy/sails into icy_coffee

afb346f

CWyrtzen assigned irlnathan Mar 23, 2015

Merge branch 'master' of github.com:balderdashy/sails into icy_coffee

3d390b7

internalfx mentioned this pull request Apr 11, 2015

Added LiveScript support #2662

Merged

internalfx added 2 commits April 11, 2015 13:15

Merge branch 'master' of https://github.com/balderdashy/sails into ic…

6e57b7d

…y_coffee Conflicts: lib/hooks/moduleloader/index.js

Add livescript support

36dde17

internalfx reviewed Apr 11, 2015
View reviewed changes

internalfx added 2 commits April 17, 2015 13:26

Merge branch 'master' of https://github.com/balderdashy/sails into ic…

5685d67

…y_coffee

Remove TODO's since I did them 😄

4d5fbf6

internalfx added 2 commits April 18, 2015 12:37

cleanup, make code DRY'er, allow easy addition of new languages

138eefb

add comments on the supported languages array

7d8d61a

fix failing test

5193ba7

CWyrtzen assigned mikermcneil and unassigned irlnathan Apr 20, 2015

mikermcneil added a commit that referenced this pull request Apr 22, 2015

Merge pull request #2599 from internalfx/icy_coffee

260eb33

Add IcedCoffeeScript support, add lazy loading for CoffeeScript and Iced...

mikermcneil merged commit 260eb33 into balderdashy:master Apr 22, 2015

sgress454 added a commit that referenced this pull request Jun 29, 2015

Fix path to Livescript module

cc39522

refs #3017, #2599

ctartist621 pushed a commit to ctartist621/sails that referenced this pull request Feb 3, 2016

Fix path to Livescript module

41179e9

refs balderdashy#3017, balderdashy#2599

balderdashy locked and limited conversation to collaborators Sep 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IcedCoffeeScript support, add lazy loading for CoffeeScript and Iced... #2599

Add IcedCoffeeScript support, add lazy loading for CoffeeScript and Iced... #2599

internalfx commented Jan 30, 2015

loicsaintroch commented Jan 30, 2015

sgress454 commented Jan 30, 2015

internalfx commented Jan 30, 2015

mikermcneil commented Jan 30, 2015

internalfx commented Jan 31, 2015

internalfx commented Jan 31, 2015

internalfx commented Feb 5, 2015

kevinburkeshyp Feb 5, 2015

internalfx commented Feb 19, 2015

internalfx commented Apr 11, 2015

internalfx Apr 11, 2015

tjwebb commented Apr 11, 2015

internalfx commented Apr 11, 2015

tjwebb commented Apr 12, 2015

internalfx commented Apr 17, 2015

tjwebb commented Apr 17, 2015

internalfx commented Apr 17, 2015

internalfx commented Apr 17, 2015

internalfx commented Apr 18, 2015

mikermcneil commented Apr 22, 2015

internalfx commented Apr 22, 2015

mikermcneil commented Sep 16, 2016

Add IcedCoffeeScript support, add lazy loading for CoffeeScript and Iced... #2599

Add IcedCoffeeScript support, add lazy loading for CoffeeScript and Iced... #2599

Conversation

internalfx commented Jan 30, 2015

loicsaintroch commented Jan 30, 2015

sgress454 commented Jan 30, 2015

internalfx commented Jan 30, 2015

mikermcneil commented Jan 30, 2015

internalfx commented Jan 31, 2015

internalfx commented Jan 31, 2015

internalfx commented Feb 5, 2015

kevinburkeshyp Feb 5, 2015

Choose a reason for hiding this comment

internalfx commented Feb 19, 2015

internalfx commented Apr 11, 2015

internalfx Apr 11, 2015

Choose a reason for hiding this comment

tjwebb commented Apr 11, 2015

internalfx commented Apr 11, 2015

tjwebb commented Apr 12, 2015

internalfx commented Apr 17, 2015

tjwebb commented Apr 17, 2015

internalfx commented Apr 17, 2015

internalfx commented Apr 17, 2015

internalfx commented Apr 18, 2015

mikermcneil commented Apr 22, 2015

internalfx commented Apr 22, 2015

mikermcneil commented Sep 16, 2016