Remove module shim for browser

[frangio]

Fixes #36

[joshgoebel]

Maintainer: After this you should build highlight.js with this repo in the extra folder and it'll generate a dist folder with a CDN ready distributable you can add to your repo for easy browser use.

[haltman-at]

My only questions here are:

1. If we're doing this should we make the other changes suggested here as well?
2. Would this be a breaking change? Would we need to increment the major version number?

Otherwise, whatever works, right? :)

[frangio]

I really can't tell if this would be a breaking change. But it does seem that we need to implement the new recommendations for highlightjs plugins.

[haltman-at]

OK, are you going to include that in this or should one of us do that separately later?

[frangio]

I see them as the same issue. I can do it tomorrow unless you're eager to ship it earlier.

[haltman-at]

No, no actual rush here. I just get antsy sometimes. :P

[frangio]

I started looking at this but I have some reservations about the setup proposed in highlightjs/highlight.js#3008 so I left a comment over there before moving forward.

[joshgoebel]

The bottom part should also be removed. All that is necessary is:

// CJS
module.exports = function solidity(hljs) {

or

// ESM
export default function solidity(hljs) {

Our build system should be happy with either. The CDN distributable will automatically include the registerLanguage. Folks using the source directly have to handle that themselves.

OR you don't have to point main at src/languages/solidity.js... you can point it at anything you like if you want to provide an interface on top of the language such as adding your own registration hooks, etc... our build process will happily look for the language in src/languages and if it's found there it doesn't care about your larger package setup.

[bcomnes]

Ran into this today. Currently floating a git fork by installing it top level with the shim removed.

"highlightjs-solidity": "github:little-core-labs/highlightjs-solidity",

[bcomnes]

Decided to switch to using patch-package. Would love to get this simple fix in.

[haltman-at]

OK, since @frangio doesn't have time anymore, I guess I'll take over this PR? I'd like to get this through. Only question is: Is this a breaking change? Will we have to bump the major version number if we do this? I don't want to merge until that's answered!

[frangio]

Thanks for taking over this. I would say the safe thing is to release a new major.

[haltman-at]

OK, I'll do that when I have some time soon, then!

[haltman-at]

The bottom part should also be removed. All that is necessary is:

// CJS
module.exports = function solidity(hljs) {

OK @joshgoebel I want to get back to this and actually get this merged... is this comment still applicable? Also, what is the "bottom part" that should be removed? Are there other changes I need to make to this? Assuming I don't want to go adding to much, and just want to get this working for people without them having to do ridiculous things?

Sorry, I realize all this has probably been answered before in some form, but reading over the discussion I find it doesn't entirely clear things up for me, so I thought I should ask again.

[joshgoebel]

Yes, the shim should not be necessary. The "preferred" way to ship a 3rd party grammar module is as a Node library and a separate web build in the dist folder. No shim is needed.

[haltman-at]

Yes, the problem is I was kind of hoping to avoid having to set up webpack or whatever other build system! Which is why I'm asking about if I can get away with just adding that "CJS" comment and other such simple things. :)

[joshgoebel]

There is no magic in any comment. I was only saying/showing that per our "spec" the only content that really should be in the file is a single default exported function.

If your following the developer docs Highlight.js does the build for you, there is no need for other build systems.

[haltman-at]

Oh! Silly me, the comments were just illustrative, not magic. Sorry, as I guess you can tell, my job is normally getting the logic right, build systems are fairly arcane to me. :)

So does that mean this can be merged then without any additional work? That would sure be nice!

[joshgoebel]

If you are using the documented build process to build your dist folder, yes.

[haltman-at]

Ah, I hadn't been following any build process, just publishing things as-is. OK, I guess whoever publishes this will have to look up and run this documented build process before publishing, then, once this is merged...?

(Sorry for the obvious questions, like I said, this sort of thing is not really my area...)

[joshgoebel]

. OK, I guess whoever publishes this will have to look up and run this documented build process before publishing, then, once this is merged...?

Typically 3rd party maintainers should work on their grammars INSIDE a HLJS checkout:

• some folder
  • highlight.js
    • extra
      • highlightjs-solidity/src/languages/*.js, etc.

Then to generate dist one merely builds CDN:

cd highlight.js
node ./tools/build -t cdn solidity

And the necessarily CDN files are dropped into your highlightjs-solidity/dist folder automagically. Add them to git, commit. For simple modules just checking them into git with a new release is more than sufficient.

[haltman-at]

OK, I'm really confused here. You're saying the dist folder should be checked into source control? Shouldn't build artifacts normally be kept out of source control, and only published, not checked in? But for HLJS languages you include it in source control for some reason?

I'm also confused by the following:

For simple modules just checking them into git with a new release is more than sufficient.

By "just checking them into git" -- checking what into git, and doing just that as opposed to what? And is this a simple module? Are you saying that I can skip any build process here, and continue just publishing with no build as I've been doing? Or no? (Because if I can, that's what I am going to do!)

Also, when you talk about working inside an HLJS checkout, would that be using git submodules? Actually I'm confused about how working inside an HLJS checkout would work when this language isn't part of the highlight.js package, but is an entirely separate package. Like, what, I just go and add this git submodule, despite the fact that that is a change to highlight.js itself, and I simply never check in that change? Or what?

[joshgoebel]

OK, I'm really confused here. You're saying the dist folder should be checked into source control? Shouldn't build artifacts normally be kept out of source control, and only published, not checked in?

Sure, if you want to setup a complex build pipeline and official releases, etc... the super easy way to do it is run the script, git add, git commit... that's what MOST all our 3rd party grammar modules do - for simplicity. So, you choose.

Are you saying that I can skip any build process here, and continue just publishing with no build as I've been doing?

I'd worry you'd be broken in the browser (since there is no module top-level variable), or vise versa, etc... usually we recommend people write their grammars with a single default export (ES6, but CJS works also) and then our build system "does the right thing" and publishes CDN ready builds... Of course the default main codebase should work in Node since it can just be required or imported.

For a perfect example (CJS):

https://github.com/highlightjs/highlightjs-robots-txt/blob/master/src/robots-txt.js

Also, when you talk about working inside an HLJS checkout, would that be using git submodules?

No, just checkout your repo into the extra folder... you're not adding it to HLJS in any way, shape, or form, just throwing it into a folder for convenience.

how working inside an HLJS checkout would work when this language isn't part of the highlight.js package, but is an entirely separate package.

Our build process treats the extra folder as special and compiles/builds/security checks grammars found there exactly as it does for built-in grammars in src/languages. This is intentional and how it's intended that 3rd parties work on and publish their grammars (without needing a complex build system of their own - you just use ours).

The automated security checks alone are a REALLY GOOD reason to use our build process.

Like, what, I just go and add this git submodule, despite the fact that that is a change to highlight.js itself, and I simply never check in that change? Or what?

Nope, extra is in .gitignore so even if you're working on HLJS patches it should just be ignored. The only reason to place it under our source tree is so that the build system can access it easily.

[haltman-at]

Oh, nice, it's in .gitignore! This all seems quite doable then. Except I'll avoid checking in build artifacts to source control; avoiding that doesn't seem like any big complication. Thanks!

[haltman-at]

OK, cool, I think I get how this works, I'll go ahead with this soon. :)

[haltman-at]

OK, time to finally pull the trigger on this!