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

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

Closed
wheresrhys opened this Issue Feb 4, 2014 · 23 comments

Comments

Projects
None yet
4 participants
@wheresrhys
Member

wheresrhys commented Feb 4, 2014

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

This comment has been minimized.

Show comment
Hide comment
@wheresrhys

wheresrhys Feb 6, 2014

Member

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

Member

wheresrhys commented Feb 6, 2014

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

@triblondon

This comment has been minimized.

Show comment
Hide comment
@triblondon

triblondon Feb 6, 2014

Contributor

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.

Contributor

triblondon commented Feb 6, 2014

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

This comment has been minimized.

Show comment
Hide comment
Contributor

triblondon commented Feb 10, 2014

@triblondon triblondon closed this Feb 10, 2014

@wheresrhys

This comment has been minimized.

Show comment
Hide comment
@wheresrhys

wheresrhys Feb 10, 2014

Member

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.

Member

wheresrhys commented Feb 10, 2014

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 triblondon reopened this Feb 10, 2014

@triblondon

This comment has been minimized.

Show comment
Hide comment
@triblondon

triblondon Feb 10, 2014

Contributor

OK, renaming this to the generic problem.

Contributor

triblondon commented Feb 10, 2014

OK, renaming this to the generic problem.

@wheresrhys

This comment has been minimized.

Show comment
Hide comment
@wheresrhys

wheresrhys Feb 13, 2014

Member

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)

Member

wheresrhys commented Feb 13, 2014

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

This comment has been minimized.

Show comment
Hide comment
@triblondon

triblondon Feb 13, 2014

Contributor

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

Contributor

triblondon commented Feb 13, 2014

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

@wheresrhys

This comment has been minimized.

Show comment
Hide comment
@wheresrhys

wheresrhys Feb 20, 2014

Member

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?

Member

wheresrhys commented Feb 20, 2014

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

This comment has been minimized.

Show comment
Hide comment
@triblondon

triblondon Feb 21, 2014

Contributor

I'll create the transform.

Contributor

triblondon commented Feb 21, 2014

I'll create the transform.

@triblondon triblondon assigned triblondon and unassigned wheresrhys Feb 21, 2014

@triblondon

This comment has been minimized.

Show comment
Hide comment
@triblondon

triblondon Feb 25, 2014

Contributor

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.

Contributor

triblondon commented Feb 25, 2014

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

This comment has been minimized.

Show comment
Hide comment
@wheresrhys

wheresrhys Feb 25, 2014

Member

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?

Member

wheresrhys commented Feb 25, 2014

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

This comment has been minimized.

Show comment
Hide comment
@wheresrhys

wheresrhys Feb 25, 2014

Member

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

Member

wheresrhys commented Feb 25, 2014

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 triblondon removed their assignment Feb 25, 2014

@triblondon

This comment has been minimized.

Show comment
Hide comment
@triblondon

triblondon Mar 15, 2014

Contributor

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

  • brfs doesn't remove require('fs') if it's assigned to a variable. require('fs').readFile() works, but fs=require('fs'); fs.readFile() will fail, because debowerify won't find fs module.
  • All paths must use __dirname, otherwise brfs assumes reading from current working directory, which is not related to any module.
  • brfs doesn't sanitize paths, so modules are able to include any file from anywhere on disk (fs.readSync('/etc/passwd') works), and even though brfs itself has a hook for inspecting file paths, browserify API doesn't expose it.
Contributor

triblondon commented Mar 15, 2014

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

  • brfs doesn't remove require('fs') if it's assigned to a variable. require('fs').readFile() works, but fs=require('fs'); fs.readFile() will fail, because debowerify won't find fs module.
  • All paths must use __dirname, otherwise brfs assumes reading from current working directory, which is not related to any module.
  • brfs doesn't sanitize paths, so modules are able to include any file from anywhere on disk (fs.readSync('/etc/passwd') works), and even though brfs itself has a hook for inspecting file paths, browserify API doesn't expose it.

@triblondon triblondon reopened this Mar 15, 2014

@triblondon

This comment has been minimized.

Show comment
Hide comment
@triblondon

triblondon Mar 17, 2014

Contributor

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

Contributor

triblondon commented Mar 17, 2014

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

@triblondon

This comment has been minimized.

Show comment
Hide comment
@triblondon

triblondon Apr 1, 2014

Contributor

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.

Contributor

triblondon commented Apr 1, 2014

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

This comment has been minimized.

Show comment
Hide comment
@triblondon
Contributor

triblondon commented Apr 10, 2014

@wheresrhys

This comment has been minimized.

Show comment
Hide comment
@wheresrhys

wheresrhys Apr 11, 2014

Member

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 substack/node-falafel#29, so using it as a dependency will cause npm shrinkwrap problems. Can we use the fork I created to solve this issue instead?

Member

wheresrhys commented Apr 11, 2014

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 substack/node-falafel#29, so using it as a dependency will cause npm shrinkwrap problems. Can we use the fork I created to solve this issue instead?

@wheresrhys wheresrhys reopened this Apr 11, 2014

@kornelski

This comment has been minimized.

Show comment
Hide comment
@kornelski

kornelski Apr 11, 2014

Contributor

I've switched falafel to your fork.

I don't mind open-sourcing it.

Contributor

kornelski commented Apr 11, 2014

I've switched falafel to your fork.

I don't mind open-sourcing it.

@wheresrhys

This comment has been minimized.

Show comment
Hide comment
@wheresrhys

wheresrhys Apr 14, 2014

Member

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

Member

wheresrhys commented Apr 14, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@kornelski

kornelski Apr 14, 2014

Contributor

Yeah, Stash is a bit weird with this:

npm install --save git+http://git.svc.ft.com:8080/scm/ot/textrequireify.git
Contributor

kornelski commented Apr 14, 2014

Yeah, Stash is a bit weird with this:

npm install --save git+http://git.svc.ft.com:8080/scm/ot/textrequireify.git
@triblondon

This comment has been minimized.

Show comment
Hide comment
@triblondon

triblondon Apr 14, 2014

Contributor

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.

Contributor

triblondon commented Apr 14, 2014

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.

@triblondon triblondon closed this Apr 14, 2014

@richard-still-ft

This comment has been minimized.

Show comment
Hide comment
@richard-still-ft

richard-still-ft Apr 24, 2014

Contributor

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.

Contributor

richard-still-ft commented Apr 24, 2014

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

This comment has been minimized.

Show comment
Hide comment
@triblondon

triblondon Apr 28, 2014

Contributor

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

Contributor

triblondon commented Apr 28, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment