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

Added warning that module names exist in URL space #2322

Merged
merged 1 commit into from Mar 8, 2018

Conversation

CxRes
Copy link

@CxRes CxRes commented Jun 28, 2017

Added a warning that module names exist in URL space, so should be / separated. In reference to systemjs/builder#814.

Added a warning that module names exist in URL space, so should be / separated. In reference to systemjs/builder#814.
```
jspm bundle app/main build.js
```

Creates a file `build.js` containing `app/main` and all its dependencies referenced in config.js.
Creates a file `build.js` containing `app/main` and all its dependencies referenced in config.js. \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the trailing /?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To force a newline! I guess what happens is that in my editor, md renders differently from github (despite being set for gfm). It should be trivial enough for you to fix :)

Copy link
Collaborator

@aluanhaddad aluanhaddad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this! Looks good to me except for the nit regarding / below

@@ -6,11 +6,12 @@ There are three main workflows for production:

### Creating a Bundle

***Important**: The module names such as `app/main` used in the examples below should only be `/` separated (On Windows, do NOT use `\` as your path separator for this argument). The module names are specified in URL space; in particular, they are not file-paths.*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be reworded slightly. It should say something closer to what the title of this PR says. Also, I think Note: would be better than Important.

@CxRes
Copy link
Author

CxRes commented Nov 2, 2017 via email

@aluanhaddad
Copy link
Collaborator

Agreed.

Copy link
Collaborator

@aluanhaddad aluanhaddad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think any rewording is necessary after all. Thank you for your contribution!

@aluanhaddad aluanhaddad merged commit 4a3134e into jspm:master Mar 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants