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

Contribute to the thought process for 3rd party language packaging if you'd like #2

Open
joshgoebel opened this issue Dec 20, 2019 · 55 comments

Comments

@joshgoebel
Copy link
Member

As a maintainer of a Highlight.js language grammar you might be interested in the discussion of an official packaging format. I just created an issue to track the discussion and I've been working on this along with the new build system.

highlightjs/highlight.js#2328

Sorry for the spam, but I couldn't think of an easier way to ping the people who might be most interested in weighing in on the subject. Feel free to simply close this issue or leave it open (whatever works best for you!).

gusbemacbe added a commit that referenced this issue Dec 24, 2019
@gusbemacbe
Copy link
Collaborator

@yyyc514

I tried to minify the file cyber-original.js, but it gave an error, due to function(hljs), but I did not see any error.

@joshgoebel
Copy link
Member Author

joshgoebel commented Dec 24, 2019

You don't want to minify it yourself - it's not JUST a minified file - it's a packaged CDN ready file, that's what the new build system is for. If you're using my branch that has the new build system and your language is in:

[highlightjs checkout]/extra/highlightjs-cyber

# from the hightlight-js parent folder
node  --stack-size=65500  ./tools/build.js -t cdn

Will build the dist file for you when it builds the CDN assets.

@joshgoebel
Copy link
Member Author

You should be exporting a module though:

module.exports = function (hljs) {

@gusbemacbe
Copy link
Collaborator

The command node --stack-size=65500 ./tools/build.js -t cdn seems not to work.

@gusbemacbe
Copy link
Collaborator

In spite of it, do the minified files seem OK to you?

@joshgoebel
Copy link
Member Author

How does it not work? Are you using the branch with the new build system?

@joshgoebel
Copy link
Member Author

joshgoebel commented Dec 24, 2019

In spite of it, do the minified files seem OK to you?

It's not a CDN module. CDN modules include the registration code so that all you have to do is include the JS tag... and NOTHING else.

@joshgoebel
Copy link
Member Author

joshgoebel commented Dec 24, 2019

What is cypher vs cypher original? If you're publishing two entirely different languages you need a slightly different structure, more like how the core library does it.

@joshgoebel
Copy link
Member Author

Here is what a CDN module looks like:

hljs.registerLanguage("bnf",(function(n){return{contains:[{className:"attribute",begin:/</,end:/>/},{begin:/::=/,starts:{end:/$/,contains:[{begin:/</,end:/>/},n.C_LINE_COMMENT_MODE,n.C_BLOCK_COMMENT_MODE,n.APOS_STRING_MODE,n.QUOTE_STRING_MODE]}}]}}));

@gusbemacbe
Copy link
Collaborator

gusbemacbe commented Dec 24, 2019

The cypher-original was not implemented by me, and does not require you to use the extra script of JavaScript code.

The cypher was implemented by me, and needs the extra script of JavaScript code.

@joshgoebel
Copy link
Member Author

joshgoebel commented Dec 24, 2019

You only need the original file, you don't need special code to make it work in a browser... that's what the new build process does for you. :-) And it will generate the dist file for you.

  • The dist file is what you'd use in the web browser.
  • The src file is what you'd use with Node/rollup/etc.

@joshgoebel
Copy link
Member Author

joshgoebel commented Dec 24, 2019

You need to be on the squash_build_pipeline branch which has the new build system. I'm hoping to get it merged in the next few weeks, but holidays and all...

@gusbemacbe
Copy link
Collaborator

OK, I created a folder tools and a file build.js (empty), and tried to run node --stack-size=65500 ./tools/build.js -t cdn, but it did not anything or minify the files.

@joshgoebel
Copy link
Member Author

joshgoebel commented Dec 24, 2019

./tools/build.js is a part of highlight.js.

You run that command from inside highlight.js. Your repository is checked out UNDER the highlight.js folder. Your repo goes in the extra folder.

Screen Shot 2019-12-24 at 1 42 48 AM

@joshgoebel
Copy link
Member Author

joshgoebel commented Dec 24, 2019

Of course your package.json also needs to point to the correct location of your src file. :-)

@gusbemacbe
Copy link
Collaborator

gusbemacbe commented Dec 24, 2019

Now I understood. But I do not like to use git@git. I prefer the HTTPS:// Is it possible?

@joshgoebel
Copy link
Member Author

You can check them out however you like... what's important is where the folders are.

@joshgoebel
Copy link
Member Author

I just fixed package.json locally and changed your main file back to doing module.exports and it builds fine with my branch and creates the dist file as expected.

@joshgoebel
Copy link
Member Author

I can make PR if you ant, but you'll still need the correct branch to run the build script.

@gusbemacbe
Copy link
Collaborator

I can make PR if you ant, but you'll still need the correct branch to run the build script.

You can make a PR now. But firstly will I need to correct the branch?

@joshgoebel
Copy link
Member Author

But firstly will I need to correct the branch?

You can't build it (the file in dist) yourself unless you're using the new branch.

@gusbemacbe
Copy link
Collaborator

gusbemacbe commented Dec 24, 2019

You can't build it (the file in dist) yourself unless you're using the new branch.

In the new branch squash_build_pipeline that I have already just created, there is not the folder dist yet.

@joshgoebel
Copy link
Member Author

joshgoebel commented Dec 24, 2019

squash_build_pipeline is a branch of highlight.js. It's not a branch you have to create.

highlightjs/highlight.js#2312

@joshgoebel
Copy link
Member Author

joshgoebel commented Dec 24, 2019

Inside highlight.js folder this might help:

git checkout squash_build_pipeline
npm install
node ./tools/build.js -t cdn

@gusbemacbe
Copy link
Collaborator

░▒▓     ~/GitHub/highlight.js     squash_build_pipeline ?1 ▓▒░·················
❯ node ./tools/build.js -t cdn
internal/modules/cjs/loader.js:775
    throw err;
    ^

Error: Cannot find module './roll_browser'
Require stack:
- /home/gusbemacbe/GitHub/highlight.js/tools/rollup_cdn.js
- /home/gusbemacbe/GitHub/highlight.js/tools/build.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:772:15)
    at Function.Module._load (internal/modules/cjs/loader.js:677:27)
    at Module.require (internal/modules/cjs/loader.js:830:19)
    at require (internal/modules/cjs/helpers.js:68:18)
    at Object.<anonymous> (/home/gusbemacbe/GitHub/highlight.js/tools/rollup_cdn.js:11:37)
    at Module._compile (internal/modules/cjs/loader.js:936:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:947:10)
    at Module.load (internal/modules/cjs/loader.js:790:32)
    at Function.Module._load (internal/modules/cjs/loader.js:703:12)
    at Module.require (internal/modules/cjs/loader.js:830:19)
    at require (internal/modules/cjs/helpers.js:68:18)
    at Object.<anonymous> (/home/gusbemacbe/GitHub/highlight.js/tools/build.js:87:13)
    at Module._compile (internal/modules/cjs/loader.js:936:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:947:10)
    at Module.load (internal/modules/cjs/loader.js:790:32)
    at Function.Module._load (internal/modules/cjs/loader.js:703:12) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/gusbemacbe/GitHub/highlight.js/tools/rollup_cdn.js',
    '/home/gusbemacbe/GitHub/highlight.js/tools/build.js'
  ]
}
zsh: exit 1     node ./tools/build.js -t cdn

@joshgoebel
Copy link
Member Author

joshgoebel commented Dec 24, 2019

That's an older copy of the branch... the latest is:

commit 3ffc2bbf2fe34d501d125c908c2d1d7014ad893a (HEAD -> squash_build_pipeline, yyyc514/squash_build_pipeline)
Author: Josh Goebel <[email protected]>
Date:   Mon Dec 23 16:56:17 2019 -0500

    wip: package CDN distributable for 3rd party languages

@joshgoebel
Copy link
Member Author

Maybe you need to git fetch --all ?

@gusbemacbe
Copy link
Collaborator

I did, but the error is still insistant. Maybe I will git-reset to your latest communit 3ffc2bbf2fe34d501d125c908c2d1d7014ad893a.

@joshgoebel
Copy link
Member Author

Yeah you might have your local branches pointed to the wrong revision. git is just instinctive to me but without being in front of your computer it's hard to tell you what to do, fetch, pull, etc... plus I don't know what remotes you have, etc. :-)

@gusbemacbe
Copy link
Collaborator

gusbemacbe commented Dec 24, 2019

But not successful yet...

git reset --hard 3ffc2bbf2fe34d501d125c908c2d1d7014ad893a
fatal: Could not parse object '3ffc2bbf2fe34d501d125c908c2d1d7014ad893a'.

I used HTTPS instead of SSH. See how I did:

git clone https://github.com/highlightjs/highlight.js
git checkout squash_build_pipeline
mkdir extra && cd extra
git clone https://github.com/highlightjs/highlight-cypher
cd ../..
node ./tools/build.js -t cdn

Did I do something wrong?

@gusbemacbe
Copy link
Collaborator

The log:

░▒▓     ~/GitHub/highlight.js     squash_build_pipeline ?1 ▓▒░·················································································································································································································
❯ git log

commit 73f0e0bdbae234b35526cd4b0d62f30981166963 (HEAD -> squash_build_pipeline, origin/squash_build_pipeline)
Author: Josh Goebel <[email protected]>
Date:   Fri Dec 6 18:51:03 2019 -0500

    rename rollup build files

I switched to master, and the log:

░▒▓     ~/GitHub/highlight.js     master ?1 ▓▒░································································································································································································································
❯ git log

commit bd545651b8bf0e6c4f8b8d37ca4af2ce311127ec (HEAD -> master, origin/master, origin/HEAD)
Author: Youssef Victor <[email protected]>
Date:   Fri Dec 20 18:22:52 2019 +0200

    (vbnet) Add missing `nameof` keyword (#2329)

@joshgoebel
Copy link
Member Author

joshgoebel commented Dec 24, 2019

What are your remotes? The latest commit for both my repo and highlightjs is 3ffc2bbf2fe34d501d125c908c2d1d7014ad893a on that branch.

@joshgoebel
Copy link
Member Author

joshgoebel commented Dec 24, 2019

Ah I have force pushed it IIRC, so you might need to reset --hard to that specific commit.

@gusbemacbe
Copy link
Collaborator

AH, I got, the official repository does not have your latest commit in both branches. Your latest commit is in your repository: https://github.com/yyyc514/highlight.js/tree/squash_build_pipeline.

@joshgoebel
Copy link
Member Author

joshgoebel commented Dec 24, 2019

Ah. Fixed. :) Too many repos. I did finally rename my remotes so now I'm my own origin. This should help in the future. :-)

@gusbemacbe
Copy link
Collaborator

Ah, and it worked. I saw cypher files were minified. Should I do anything else?

@joshgoebel
Copy link
Member Author

Screen Shot 2019-12-24 at 2 43 42 AM

You don't need the two files in root anymore.

@joshgoebel
Copy link
Member Author

Your README probably need to be updated to follow the new conventions. See robots-txt repo for an example.

@gusbemacbe
Copy link
Collaborator

@yyyc514

Ready.

@joshgoebel
Copy link
Member Author

You shouldn't include this unless you're actually going to publish your package to NPM.

<script type="text/javascript"
  src="https://unpkg.com/highlightjs-cypher@master/dist/cypher.min.js"></script>

You should change the RobotsTxt variable name:

var hljsRobotsTxt = require('highlightjs-cypher');

@gusbemacbe
Copy link
Collaborator

You shouldn't include this unless you're actually going to publish your package to NPM.

Actually I published to NPM.

@gusbemacbe
Copy link
Collaborator

Ready.

@joshgoebel
Copy link
Member Author

@gusbemacbe
Copy link
Collaborator

Fixed, @yyyc514.

@joshgoebel
Copy link
Member Author

Looking pretty good. Still not sure where your markup expect test files are. Did you just not commit them?

@gusbemacbe
Copy link
Collaborator

Ah, you have removed the expect test files. I will check the old commits to download the old files.

@gusbemacbe
Copy link
Collaborator

I have just added the missed file sample.expect.txt, and if you need more expect files, let me know.

@gusbemacbe
Copy link
Collaborator

Anything else, @yyyc514 ?

@joshgoebel
Copy link
Member Author

joshgoebel commented Dec 24, 2019

I'm not sure you're understanding how it works. For markup tests you need TWO files for each test:

  • keywords.txt (the raw input to Highlight.js)
  • keywords.expect.txt (the correct HTML output that SHOULD be generated if your code is working properly)

You should be able to run the test suite from inside the highlight.js directory and see the failing tests because it will yell at you that it can't find the files.

The custom test scaffold you write LOOKs like you understood the system, but then the files aren't there - so your own tests would never have passed (that I can see).

@gusbemacbe
Copy link
Collaborator

Hello @yyyc514 !

I am very sorry for my long absence. The company overcharged me with a lot of things.

  • keywords.txt (the raw input to Highlight.js)

  • keywords.expect.txt (the correct HTML output that SHOULD be generated if your code is working properly)

The neo4j/Cypher is not inside HTML. It is inside PHP/JavaScript/JSON on a HTML file.

@joshgoebel
Copy link
Member Author

Pretend I said "the correct output"... the test files are just:

  • expected intput
  • expect output

@gusbemacbe
Copy link
Collaborator

@yyyc514 , I think @Mogztter has already done it. See the latest commit and if those test files are what you want exactly.

@joshgoebel
Copy link
Member Author

Yeah that looks like the idea.

@gusbemacbe
Copy link
Collaborator

@yyyc514

The repository is ready.

@AdamRaichu
Copy link

Suggest closing?

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

No branches or pull requests

3 participants