Skip to content

Remove pragmas, rely on identifier instead#2

Merged
threepointone merged 1 commit intothreepointone:masterfrom
codemix:no-pragma
Nov 21, 2016
Merged

Remove pragmas, rely on identifier instead#2
threepointone merged 1 commit intothreepointone:masterfrom
codemix:no-pragma

Conversation

@phpnode
Copy link
Copy Markdown
Contributor

@phpnode phpnode commented Nov 21, 2016

It's fragile and precarious to rely on a particular identifier name for this kind of thing. It's too easy to call an identifier markdown some time in the future and find yourself fighting a bug that seems totally impossible. This PR scraps the idea of pragmas and relies on the name of the identifier being imported or required instead.

@threepointone threepointone merged commit 5ee50c3 into threepointone:master Nov 21, 2016
@threepointone
Copy link
Copy Markdown
Owner

This is great! I'll remove the bit with self linking and use a webpack resolve alias instead. Thanks much!

@threepointone
Copy link
Copy Markdown
Owner

merged and published in 1.1.3. Thank you very much!

@phpnode
Copy link
Copy Markdown
Contributor Author

phpnode commented Nov 21, 2016

Cool, it's worth noting that this is not totally foolproof, the following should work:

var foo = require('markdown-in-js') // ok
var bar;
bar = require('markdown-in-js') // ok

the following will cause pain

var foo = require('markdown-in-js') // ok
function demo () {
  var foo = 123
  console.log(foo) // boom because we don't know that foo is shadowed.
}

and

var foo = require('markdown-in-js') // ok
foo = 123
console.log(foo) // oh dear, we don't know that foo has been reassigned

It's not hard to fix this issue but it's worth adding a note in the README in the meantime

@threepointone
Copy link
Copy Markdown
Owner

threepointone commented Nov 21, 2016

yes, it's also why I went with a simple hardcoded value in the first place, just needed something that worked. I'll also make the error more meaningful in the above cases.

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.

2 participants