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

CLI: Setting sourceRoot option removes folder structure #3075

Closed
cr0 opened this issue Jul 19, 2013 · 10 comments
Closed

CLI: Setting sourceRoot option removes folder structure #3075

cr0 opened this issue Jul 19, 2013 · 10 comments
Labels

Comments

@cr0
Copy link

cr0 commented Jul 19, 2013

Hi there

is it on purpose that setting the sourceRoot option removes the coffee files' folder structure from the generated source map?

With sourceRoot set to coffee/

./assets/coffee/lib/utils.coffee

the generated source map at ./public/js/lib/utils.map will be:

"sourceRoot": "/coffee/",
"sources": [
     "utils.coffee"
]

This makes Chrome unable to find the attached coffee file for a javascript file. As you can see on the attached file the old folder structure is completely removed.
flat-structure
So Chrome reconstructs the folder structure based on the source maps wrong. This makes it impossible for me to debug things because every request for a coffee file in a subfolder ends in a 404.

I'm using grunt to build:

coffee:
  client:
    options:
      sourceMap: true
      sourceRoot: '/coffee/'
    files: [
      expand:   true
      cwd:      'assets/coffee/'
      src:      ['**/*.coffee']
      dest:     'public/js/'
      ext:      '.js'
    ]

Is something wrong with my build setup? Or is this a bug (or a feature ;))? I'm on 1.6.3.

@jashkenas
Copy link
Owner

@jwalton -- got any ideas about this one?

@jwalton
Copy link

jwalton commented Oct 21, 2013

I'll have a look tomorrow morning.
On Oct 20, 2013 3:36 PM, "Jeremy Ashkenas" notifications@github.com
wrote:

@jwalton https://github.com/jwalton -- got any ideas about this one?


Reply to this email directly or view it on GitHubhttps://github.com//issues/3075#issuecomment-26680635
.

@jwalton
Copy link

jwalton commented Oct 21, 2013

When you run the coffee command (from command.coffee) we work out the relative path from each js file to each source file, and explicitly set sourceRoot for each file as we pass each file to the compiler. If you run:

coffee -c -m -o lib src

Then you end up with files like:

"sourceRoot": "../..",
"sources": [
"src/foo/bar.coffee"
],

Changing your CWD will change these paths. The compiler, though, just blindly passes sourceRoot into sourcemap.litcoffee for each individual file.

So, why work out the relative paths in command.coffee instead of simply letting the compiler work out the relative paths on its own in coffee-script.coffee? The problem is that command.coffee does all the file handling logic. By the time we get into coffee-script.coffee, all we have is a string containing coffee source code and the options.

I'm not entirely sure what the best way to fix this is. The most immediate fix is to pass this off to @tkellen from @gruntjs, as they should be computing the sourceRoot here the same way we do here.

But, to be honest, it seems like we could be handling this better. We do pass in the jsPath as an option to the compiler, so we could derive the paths in coffee-script.coffee from and the sourceFiles... Although, at a glance I notice that gruntjs doesn't pass in jsPath. :P

I wonder if we should provide a better API for compiling files? It seems like we should have a:

require('coffee-script').compileFiles(sourceFileOrFiles, destFile, {sourceMaps: true, ...});

that takes care of all of this for you, and which itself gets called from command.coffee. This would be a bit more friendly an API for grunt and other tools like it, which otherwise have to duplicate a lot of the file handling infrastructure that coffee has already built. Thoughts @jashkenas?

@jashkenas
Copy link
Owner

In a perfect world, that sounds lovely -- but I'm afraid that coffee-script.coffee is supposed to be the "pure" interface to the compiler, that can be run on Node, or in a browser, where there's no such thing as file paths and reading from the disk.

But I'm totally open to an approach that preserves that characteristic while providing a nicer API along the lines of what you're proposing.

@jwalton
Copy link

jwalton commented Oct 21, 2013

WRT having coffee-script.coffee figure things out from jspath, yes, you're correct, although we could write our own js implementation of path.relative(...) used when we're running outside of node (although... ugly.)

WRT to my proposed compileFiles() - this doesn't have to live in coffee-script.coffee; it could easily go somewhere else. It just needs to be exported via the index for node clients.

@jashkenas
Copy link
Owner

Ah ha -- that sounds just fine then. There's actually precent for reimplementing a bit of the 'path' module for the browsers in helpers.coffee already -- baseFileName

@jashkenas jashkenas reopened this Oct 21, 2013
@epidemian
Copy link
Contributor

The other day i was playing with UglifyJS's source map generation, and, although it has quite a few parameters to configure the paths, getting them right was not so difficult (taking a look at what the browser was requesting), and after that i felt the build process was quite solid.

I think that for a general tool (like a compiler or a minifier) it's better not to try and do too much guessing magic; especially about paths and directory structure. I personally would prefer having some simple default behaviour and let the user override it in case it's not adequate for their build process (even if means adding a couple of source-map specific options) rather than having a convoluted logic for this file path generation.

Obviously, if there's a simple solution that always gets these paths right and it doesn't require any extra command line options, then that'd be fantastic.

@GeoffreyBooth GeoffreyBooth changed the title Setting sourceRoot option removes folder structure CLI: Setting sourceRoot option removes folder structure May 6, 2017
@GeoffreyBooth
Copy link
Collaborator

Is this still an issue?

@GeoffreyBooth
Copy link
Collaborator

So I looked into this a little. Using the folder structure from #3296, I wrote a script to compile that .coffee file (because sourceRoot is only an API option, not a command-line option):

fs = require 'fs'
CoffeeScript = require 'coffeescript'

filename = './website_root/coffee/work.coffee'

{ js, v3SourceMap, sourceMap } = CoffeeScript.compile fs.readFileSync(filename, 'utf-8'),
	filename: filename
	bare: yes
	sourceMap: yes

console.log v3SourceMap

And it printed this (2.0.0-beta4):

{
  "version": 3,
  "file": "",
  "sourceRoot": "",
  "sources": [
    ""
  ],
  "names": [],
  "mappings": "AAAA,OAAO,CAAC,GAAR,CAAY,IAAI,CAAC,GAAL,CAAA,CAAZ"
}

Well well well, not at all what we want! Empty sources and sourceRoot?

Looking into sourcemap.litcoffee, I found this:

Produce the canonical JSON object format for a "v3" source map.

        v3 =
          version:    3
          file:       options.generatedFile or ''
          sourceRoot: options.sourceRoot or ''
          sources:    options.sourceFiles or ['']
          names:      []
          mappings:   buffer

So apparently, if you pass the undocumented sourceRoot and sourceFiles in as options, they get output as is into the source map. And indeed, changing my script:

{ js, v3SourceMap, sourceMap } = CoffeeScript.compile fs.readFileSync(filename, 'utf-8'),
	filename: filename
	bare: yes
	sourceMap: yes
	sourceRoot: './website_root/coffee/'
	sourceFiles: [filename]

produced:

{
  "version": 3,
  "file": "",
  "sourceRoot": "./website_root/coffee/",
  "sources": [
    "./website_root/coffee/work.coffee"
  ],
  "names": [],
  "mappings": "AAAA,OAAO,CAAC,GAAR,CAAY,IAAI,CAAC,GAAL,CAAA,CAAZ"
}

So the original bug above is no longer reproducible, or perhaps was a bug in Grunt or grunt-coffee. So we can close this issue.

But as for the empty strings, we have options:

  1. Leave the code alone and just document sourceRoot and sourceFiles in the Node API docs.
  2. Document sourceRoot, but generate sources from [filename] (since the compile method takes a string as input, so by definition you’re only compiling one file) if sourceFiles isn’t specified but filename is.

Also should the default values for these really be null? @lydell, I see you have some history with this: mozilla/source-map#123

GeoffreyBooth added a commit to GeoffreyBooth/coffeescript that referenced this issue Aug 30, 2017
…t `filename` is, use `filename`; output null instead of an empty string for `sources` or `sourceRoot`
GeoffreyBooth added a commit to GeoffreyBooth/coffeescript that referenced this issue Aug 30, 2017
…t `filename` is, use `filename`; output null instead of an empty string for `sources` or `sourceRoot`
GeoffreyBooth added a commit that referenced this issue Sep 1, 2017
* Per discussion in #3075: if `sourceFiles` is unspecified, but `filename` is, use `filename`; output null instead of an empty string for `sources` or `sourceRoot`

* Update source map tests to reflect that now we return null instead of empty strings; check generated sources array

* Update source map documentation; still leave more obscure options undocumented

* Follow the TypeScript compiler’s example regarding v3SourceMap, but output empty strings instead of made-up filenames

* Have `sources` default to ‘<anonymous>’
@GeoffreyBooth
Copy link
Collaborator

Fixed via #4671.

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

No branches or pull requests

5 participants