Skip to content
This repository was archived by the owner on May 22, 2025. It is now read-only.

Replace all slashes with periods in the module name instead of just the first #154

Merged
merged 2 commits into from
Jun 9, 2016

Conversation

jjudd
Copy link
Contributor

@jjudd jjudd commented Jun 9, 2016

No description provided.

@evmar
Copy link
Contributor

evmar commented Jun 9, 2016

Can you ensure a test covers this? I think perhaps the best option might be to move the pathToModuleName found in tsickle_test.ts out into a place where main.ts could also call it, so that those tests would cover it.

(Another way of saying this is that all the code in main.ts is currently not covered by tests, so moving code out from there and into other files will improve test coverage.)

@jjudd
Copy link
Contributor Author

jjudd commented Jun 9, 2016

Can do.

@jjudd
Copy link
Contributor Author

jjudd commented Jun 9, 2016

I have found a couple different versions of this function. Curious which one would be preferred? Or should they be combined?

function pathToModuleName(context: string, fileName: string): string {
  if (fileName[0] === '.') {
    fileName = path.join(path.dirname(context), fileName);
  }
  return fileName.replace(/\.js$/, '').replace(/\//g, '.');
}


function pathToModuleName(context: string, fileName: string) {
  if (fileName[0] === '.') {
    fileName = path.join(path.dirname(context), fileName);
  }
  return fileName.replace(/\//g, '$').replace(/_/g, '__');
}

@evmar
Copy link
Contributor

evmar commented Jun 9, 2016

It doesn't matter too much. In a pure-TS app, the module names disappear after the closure compile. It could matter if you import a TS module into a Closure JS module (you need to know what the name is).

The former one seems better to me.

@jjudd
Copy link
Contributor Author

jjudd commented Jun 9, 2016

I went with the first function and used it in both the CLI and the tests. I'm not sure what the naming convention is and naming things is just hard in general. If we want to rename things, I'm totally fine with that.

@evmar
Copy link
Contributor

evmar commented Jun 9, 2016

Thanks for the patch!

@evmar evmar merged commit 9d641fb into angular:master Jun 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants