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

set writeToDisk to false by default #365

Closed
aaronshaf opened this issue Sep 12, 2016 · 18 comments
Closed

set writeToDisk to false by default #365

aaronshaf opened this issue Sep 12, 2016 · 18 comments
Labels
blocked Cannot be completed before another issue. The blocking issue should be linked. proposal scope:compiler status:backlog Tasks planned to be worked on type:feature A feature request

Comments

@aaronshaf
Copy link

aaronshaf commented Sep 12, 2016

As services like https://zeit.co/now become more ubiquitous, and writing to disk in an app server becomes less kosher, this seems like a better option.

I'm having to manually load/pass-through my includes right now just so I can set writeToDisk to false, and having to set writeToDisk adds a bit more noise to my code.

My 2c. :)

@patrick-steele-idem
Copy link
Contributor

There are a few reasons we have kept writeToDisk enabled:

  • In development, seeing the output .marko.js file is helpful for debugging and understanding
  • In production, templates are not recompiled if the application server needs to restart since the compiled out from the previous startup was written to disk. This makes startup times slightly faster

Since disabling writeToDisk is as simple as require('marko/compiler').configure({ writeToDisk: false }) I'm in favor of keeping the current behavior. I'm open to other thoughts though.

@mlrawlings @philidem any thoughts?

@patrick-steele-idem patrick-steele-idem added the type:feature A feature request label Oct 5, 2016
@hustcer
Copy link

hustcer commented Oct 7, 2016

@patrick-steele-idem Do we have to compile marko files first in order to serve them dynamically?

@patrick-steele-idem
Copy link
Contributor

@hustcer templates should always be compiled on the server. When you call require('./template.marko') on the server the template will be compiled and loaded at that time. When you use a JavaScript module bundler to send down templates they will be compiled during the bundling process. If that doesn't answer your question I would recommend joining us on Gitter to discuss: https://gitter.im/marko-js/marko

@hustcer
Copy link

hustcer commented Oct 7, 2016

@patrick-steele-idem I see, thanks.

@Hesulan
Copy link
Contributor

Hesulan commented Dec 2, 2016

@patrick-steele-idem I think I'm in favor of making false the default. The compiled output is extremely useful for the things you mentioned and many others, but 98% of the time it just wastes disk I/O (which is a very bad thing if you develop on cloud storage or flash drives - which I do).

On the other hand, it's extremely easy to en-able writeToDisk when you need to debug something (not as convenient as having the file already there, but still super easy), and anyone concerned with start-up time should really just pre-compile everything in their build step and never require() the compiler at all during production.

@sandro-pasquali
Copy link
Contributor

FWIW the surprise appearance of the compiled template as a .js file can disrupt the typical file watching/rebuild patterns in dev environments by firing off a misleading file/directory change event. This is another thing that is easy to fix (set ignore rules), but... Indeed, in dev you probably don't even want compilation caching to happen at all (requiring even more little fixes), and you'll have to add marko.js files to your .gitignore sooner rather than later...

Easy fixes, but now a little pile of them :)

All that said, Marko is awesome and I think helping new users to avoid these surprises can only be a good thing.

@mlrawlings
Copy link
Member

I would agree that it would be nice to remove these files by default if only because it makes things seem simpler. I will say though, that these compiled output files were extremely helpful in understanding how marko works at a lower level - though that's certainly not a requirement in order to use marko.

The other concern I have is that runtime errors will reference a line/column in the generated *.marko.js file, so if that file is not readily available, debugging becomes more cumbersome.

Potential solutions:

  • sourcemaps
  • write out the generated file on error

@Hesulan
Copy link
Contributor

Hesulan commented Dec 17, 2016

@mlrawlings I completely agree that the *.marko.js files are essential for debugging, but in my opinion it's already easy enough to generate them when desired with ./node_modules/.bin/markoc ./path/to/template.marko, or even require('marko/compiler').defaultOptions.writeToDisk = true; for those who don't need to worry about unnecessary disk writes.

Writing out on error may be more convenient, but it would also result in files sometimes appearing with little or no explanation and littering the project folders, which could confuse new users even more.

I would be very interested in source maps, though I vaguely remember seeing discussion in an issue somewhere about them being too difficult to implement.

@mlrawlings
Copy link
Member

mlrawlings commented Dec 18, 2016

@Hesulan Regarding source maps, our parser already has line/column start/end positions for every tag/attribute/etc. so that's definitely a good starting point and I'm thinking with the changes made to the way we generate code in v4, it might not be too difficult to get the code writer to map changes back to the positions of the nodes its writing. Then we would need some kind of way to track where newly generated nodes came from. This would probably require the core transforms and user-generated transforms to give the compiler hints about what it's doing. This would be especially true when custom parsing tag/attribute argument expressions.

I actually just recently added a _currentNode property to the context, so if we connect the builder to use that we might be able to track things in many cases, but I there would still be cases where you might need to pass in an original node to a builder method, possibly with position offsets.

I definitely think we can get this to work. We just need the time to spend on it.

In the short-term, maybe controlling writeToDisk via an environment variable would be easiest:

MARKO_WRITE_TO_DISK=true npm start

or something more generic like:

DEBUG=marko npm start

@Hesulan
Copy link
Contributor

Hesulan commented Dec 18, 2016

@mlrawlings I really like the idea of using an environment variable, especially since the output files are only really used for production and debugging, and anyone worried about production start-up time should probably be compiling during a build step instead.

And I'd love to see the compiler keep better track of where the code came from. That might also help with things like #247.

@gilbert
Copy link
Contributor

gilbert commented Dec 30, 2016

The environment variable might have too great an effect. My personal primary use case is inspecting the generated file of only the template I'm working with. The environment variable feature would generate all files in the project, wouldn't it?

@Hesulan
Copy link
Contributor

Hesulan commented Dec 30, 2016

@mindeavor I'm thinking the environment variable would probably just switch writeToDisk on, which is currently Marko's default behavior.

If you need to compile only a specific template, you can either execute

./node_modules/.bin/markoc ./path/to/template.marko

in a shell, or toggle writeToDisk on while loading that template for the first time:

require('marko/compiler').defaultOptions.writeToDisk = true;
var template = require('./path/to/template.marko');
require('marko/compiler').defaultOptions.writeToDisk = false;

I'm not aware of any hooks or methods in the API to write a specific template to disk, but that's probably worth considering even if the default value of writeToDisk doesn't change. Something like Template.prototype.writeToDisk(), perhaps? I might look into implementing that sometime if I get the chance.

@gilbert
Copy link
Contributor

gilbert commented Dec 30, 2016

I see. Personally I think it'd be great if you could add something to the code like:

<script>
module.exports = {
  debug: true,
  onInput(attrs) {
    // ...
  }
}
</script>

where debug can be true or, if need be, an options object like { writeToDisk: true }. I would find that to be really handy :)

@patrick-steele-idem
Copy link
Contributor

That's an interesting thought @mindeavor. It's a little more verbose, but a tag such as one of the following would be easier to implement::

<write-to-disk/>
<!-- or -->
<marko-compiler write-to-disk/>
<!-- or -->
<marko-debug/>
<!-- or -->
<marko-compiler debug/>

<div>
  Hello ${data.name}
</div>

@Hesulan
Copy link
Contributor

Hesulan commented Dec 30, 2016

@patrick-steele-idem That's exactly what I was just thinking. Perhaps this would be a good first use case for the <debug> tag that was being discussed not too long ago, like <debug write-to-disk=true />?

@gilbert
Copy link
Contributor

gilbert commented Dec 30, 2016

Yes, now that you mention it, a tag would be better. That way you can debug whether or not you have a <script> tag.

@patrick-steele-idem
Copy link
Contributor

Blocked by sourcemap support: #868

@DylanPiercey
Copy link
Contributor

This is now the case in Marko 5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Cannot be completed before another issue. The blocking issue should be linked. proposal scope:compiler status:backlog Tasks planned to be worked on type:feature A feature request
Projects
None yet
Development

No branches or pull requests

8 participants