Financial-Times / ft-origami

The Old Origami Website, do not use
https://github.com/Financial-Times/origami
76 stars 12 forks source link

Solution to enable templates to be compiled into JS bundles #110

Closed wheresrhys closed 10 years ago

wheresrhys commented 10 years ago

Say I have a module that needs to use a template to render itself using client-side js. I can't load the template using ajax from a path built using o-asset because typically it will breach the same origin policy. Embedding the template inline in the page is also problematic as now we're saying product developers aren't advised to use origami templates. The only general solution I can see is require()ing them in the js, which will need https://npmjs.org/package/stringify to be part of the build.

Are there any other browserify transforms we should whitelist too https://github.com/substack/node-browserify/wiki/list-of-transforms?

wheresrhys commented 10 years ago

Just noticed also that <script> tags are explicitly prohibited from templates too

triblondon commented 10 years ago

Stringify seems like a sensible approach. I don't want to make the build process more complex, but it seems like a decent solution. We'll need to add that to the build service, but in the meantime you could make it part of the module's gruntfile and devdependencies.

triblondon commented 10 years ago

https://redmine.labs.ft.com/issues/23822

wheresrhys commented 10 years ago

I was about to implement this in the responsive article but it turns out that stringify can't be used as a cli transform in the browserify build. It's an npm module that must, along with browserify itself, be required (and configured to a degree) within the module's javascript... which breaches our use of bower for package management. I think we'll probably need to roll our own template requirer.

triblondon commented 10 years ago

OK, renaming this to the generic problem.

wheresrhys commented 10 years ago

https://github.com/unfold/browserify-hogan could easily be adapted to do what we need (it requires text files and compiles to templates, whereas we only need the first step)

triblondon commented 10 years ago

Wow, that's pretty simple. OK. Could you whip that up?

wheresrhys commented 10 years ago

As this'll be a dependency of build service and needs to be set up as an NPM module with an owner wouldn't it make sense for @pornel to set this up from the outset?

triblondon commented 10 years ago

I'll create the transform.

triblondon commented 10 years ago

I went to write this and then found https://github.com/substack/brfs which actually seems like a better solution - rather than transforming require it transforms fs.readFileSync.

wheresrhys commented 10 years ago

Great - I knew there had to be something in the browserify arsenal somewhere. I'll start using it

@dansearle-ft - have you started reworking grunt-origami-demoer? can you add this transform?

wheresrhys commented 10 years ago

hold on a second... we'll have to require('fs') - what'll be the impact of that?

No impact http://stackoverflow.com/questions/16640177/browserify-with-requirefs

triblondon commented 10 years ago

Kornel's been working on adding this, and raised the following issues:

triblondon commented 10 years ago

Kornel and I made the executive decision to drop brfs and write a new transform that just works on require(path);

triblondon commented 10 years ago

Update: To avoid conflicts with debowerify, we've decided to change the syntax slightly:

requireText('somepath')

Will now be used for string includes, where require will be used for module includes. Avoids overloading require in an unsafe way.

triblondon commented 10 years ago

This is now implemented:

http://git.svc.ft.com:8080/projects/OT/repos/textrequireify/browse

wheresrhys commented 10 years ago

Yay! But is there any reason why we're not open-sourcing this? I'd say it's a perfect candidate to be added to the npm registry & browserify README.

Also substack is ignoring the fact he's the sole maintainer of falafel and is not fixing this issue https://github.com/substack/node-falafel/pull/29, so using it as a dependency will cause npm shrinkwrap problems. Can we use the fork I created to solve this issue instead?

kornelski commented 10 years ago

I've switched falafel to your fork.

I don't mind open-sourcing it.

wheresrhys commented 10 years ago

@pornel - How are you consuming this as a dependency. I can't figure out what the url is to get the tarball for npm

kornelski commented 10 years ago

Yeah, Stash is a bit weird with this:

npm install --save git+http://git.svc.ft.com:8080/scm/ot/textrequireify.git
triblondon commented 10 years ago

On open sourcing - no objection, but I'm in no desperate rush to do so. A proper open source release should go on the ftlabs github account and needs the appropriate licence and copyright info. Not in scope for this issue anyway.

richard-still-ft commented 10 years ago

Currently the developer guide assumes this is in the NPM repository. Can we either add it to NPM or alter the documentation? I am happy to do the latter.

triblondon commented 10 years ago

Alter the docs, and if you wouldn't mind that would be much appreciated.