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

sails-linker modifies views in the source repository? #2007

Closed
simoami opened this issue Jul 23, 2014 · 12 comments
Closed

sails-linker modifies views in the source repository? #2007

simoami opened this issue Jul 23, 2014 · 12 comments

Comments

@simoami
Copy link

simoami commented Jul 23, 2014

It appears that grunt-sails-linker is configured to modify views in the source code as this configuration implies.

'sails-linker': {

            devJs: {
                options: {
                    startTag: '<!--SCRIPTS-->',
                    endTag: '<!--SCRIPTS END-->',
                    fileTmpl: '<script src="%s"></script>',
                    appRoot: '.tmp/public'
                },
                files: {
                    '.tmp/public/**/*.html': jsFilesToInject,
                    'views/**/*.html': jsFilesToInject,  <------ here
                    'views/**/*.dot': jsFilesToInject     <------ here
                }
},

This is generally not recommended. Code that is versioned will be flagged as modified. If one performs a merge from another branch and wants to test that the app is running before committing the merged code, then this will interfere with the process, since the sails-linker turns views as modified.

@edy
Copy link
Contributor

edy commented Jul 23, 2014

this is why i removed linker from my gruntfile

@simoami
Copy link
Author

simoami commented Jul 23, 2014

I'm also looking at an alternative asset manager that supports groups and CDN. Groups because I don't want to dump 3MB of javascript (ExtJS) on the login page. Started looking into assetmanager but very early to come to a conclusion.

A recommendation to @mikermcneil is to revisit the current approach for embedding assets. it's not well thought through. The sails-linker would be useful if:

  • it can operate on views and templates in the temporary folder
  • account for groups. e.g. login, multi-page apps, multi-module apps.
  • fingerprint minified resources?
  • CDN support. minified vs non-minified urls.

@edy
Copy link
Contributor

edy commented Jul 23, 2014

something what i am looking for is a nice solution to rev (fingerprint?) the assets (js, css, also relative paths inside the css, images) (read: cache busting)

@simoami
Copy link
Author

simoami commented Jul 23, 2014

For css embedded images, see here. There are definitely other alternatives.

@sgress454
Copy link
Member

You don't have to use this feature at all. You can simply remove the <!--SCRIPTS--> and <!--SCRIPTS END--> tags from your views/layout.ejs (or wherever you may have them) and you'll still get the benefit of the other asset management features (such as copying static assets to your .tmp folder).

That being said, the linker can be immensely helpful for development. I've gotten into the habit of not checking in my layout.ejs file when the only changes are due to the linker, but even if it does get checked in with those changes, they'd be overwritten by the linker the next time sails lift ran anyway.

@simoami
Copy link
Author

simoami commented Jul 24, 2014

I'm not sure why this was closed. Back to the root of the problem, The sails-linker modifies source files and it shouldn't. this needs to be fixed. Sails sells itself as being production ready and this is not an issue to ignore.

Please fix the gruntfile to operate on temporary files only and have a configuration setting to point to views in the temporary folders.

@simoami
Copy link
Author

simoami commented Jul 24, 2014

Also, there's great recommendations here in this thread and we simply chose to ignore them?

@albertosouza
Copy link

@simoami sails-linker is optional.
If you need something dynamic on production env use one variable and if you don't need only don't use it
sails-linker helps a lot how are starting with sails.

@simoami
Copy link
Author

simoami commented Jul 24, 2014

Thanks @albertosouza and @sgress454.

sails-linker helps a lot how are starting with sails.

yes, understood. I'm assessing Sails.js for a possible migration from an Enterprise java application to Node.js. Our requirements are higher than the average single page app scenario. we have a multi-module app with 600 JS files and using ExtJS as a JS framework. We also have guidelines that build tasks should never modify source code but may operate on temporary folders. For example, Maven generates target/ folders and these are excluded from versioning. So I thought our use-case is a real one. Having said that, I just thought that even though sails-linker is optional, which is great, we'd want to follow some industry standards and best practices.

@rishabhmhjn
Copy link

I tried to find a workaround to this issue by copying the views/**.*.ejs to .tmp/views and updated the sails-linker tasks to modify the files in the .tmp/views/** and its doing it well.

Now, I wanted to change the sails.config.paths.views to ./tmp/views.
I found 2 problems

  1. Adding a config/paths.js with the following did not update the sails.config.paths.views value

    module.exports.paths = {
      views: '/path/to/app/.tmp/views'
    }
    
  2. I tried to modify the sails.config.paths.views value to .tmp/views in the config/bootstrap.js and it updates the config fine. However, if I open the pages, it still does not select the files in the .tmp/views folder even though the logs says otherwise

    silly: Serving view at rel path:  homepage
    silly: View root:  /path/to/app/.tmp/views
    verbose: Rendering view: "homepage" (located @ "/path/to/app/.tmp/views/homepage")
    verbose: • using configured layout:: layout (located @ "/path/to/app/.tmp/views/layout"
    

BUG?

PS. Setting the sails.config.paths.views worked fine in sails 0.9.9

Update

solved by adding the following to the bootstrap.js

 sails.config.paths.views = sails.config.appPath + '/.tmp/views';
 sails.hooks.http.app.set('views', sails.config.paths.views);

@robgrimball
Copy link

While you can remove the linker tags/injection of source files and do it manually yourself, we tend to use the linker even for bringing in scripts for production environment. Our main problem with this approach was the lack of versioning in sails resources because of use of the linker. In case someone else is doing the same, but needs versioning of their sails resources brought in by the linker, the following stackoverflow question helped point me in the right direction, particularly the link in the second answer :

http://naya.com.np/@developer.naya/post/391862c0520b5b5632e99e812749a85b

@animedbz16
Copy link

I agree with @simoami here. This is an issue @sgress454.

There is no reason why Sails should be configured out of the box to have grunt / sails linker modify the actual source template as it makes is confusing as git sees this as a change and users will likley just commit the file over and over which is unnecessary.

The dynamic linking is important and definitely useful, so I don't think that disabling it all together is a "solution".

Sails should just be a bit smarter to see if someone who has grunt enabled, sails linker should modify the copied files in the .tmp directory as @rishabhmhjn is suggesting. This way the source file doesn't get modified, only the grunt built file is.

Though there is another use case where what if grunt is disabled say for a production server. We use the build/buildProd task to build a www static path. While we have nginx set up to serve out these files statically, I'm not sure if modifying the bootstrap.js file to point to the .tmp directory would work in the case where grunt is disabled entirely and there is no .tmp directory as maybe this would crash sails, I'm not sure as we haven't tested it.

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

No branches or pull requests

7 participants