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

fix(dingus): setup added for windows for running dingus locally #376

Closed
wants to merge 2 commits into from

Conversation

nishihere19
Copy link

@nishihere19 nishihere19 commented Apr 4, 2021

Closes #375

Added a file similar to build_dingus.js for setting up a demo of dingus on windows. On running the script dingus:win, the separate file for setting up a demo on windows will be executed.

Changes

  • Added separate script for running dingus on windows in package.json
  • A separate file including shell commands for setup on Windows.

Flags

Screenshots or Video

package json - Projects - Visual Studio Code 02-04-2021 14_27_16 (2)
slateToCiceroMarkDom js - Projects - Visual Studio Code 04-04-2021 15_57_35 (2)
templatemark dingus - Google Chrome 04-04-2021 15_59_51 (2)

Related Issues

Author Checklist

  • Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
  • Vital features and changes captured in unit and/or integration tests
  • Commits messages follow AP format
  • Extend the documentation, if necessary
  • Merging to master from fork:branchname

@coveralls
Copy link

coveralls commented Apr 4, 2021

Coverage Status

Coverage remained the same at 89.087% when pulling 5579aaf on nishihere19:develop into d776a0a on accordproject:master.

@nishihere19 nishihere19 changed the title fix(dingus): setup added for windows for running dingus locally - #375 fix(dingus): setup added for windows for running dingus locally #375 Apr 4, 2021
@nishihere19 nishihere19 changed the title fix(dingus): setup added for windows for running dingus locally #375 fix(dingus): setup added for windows for running dingus locally Apr 4, 2021
@jeromesimeon jeromesimeon self-requested a review April 4, 2021 16:39
Copy link
Member

@jeromesimeon jeromesimeon left a comment

Choose a reason for hiding this comment

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

hi @nishihere19 this is a great contribution! I can't easily test myself on Windows, but I see from the snapshot that it does seem to fix the dingus for Windows.

A few suggestions:

  1. from a local test, it seems like your new script works on my machine too (MacOS) so I would suggest to simply replace the current script by your version instead of creating an alternative one
  2. Could you use JS template strings rather than string concatenation (i.e., `node ${demodata} ... ` rather than 'node '+demodata+' ...')
  3. You will have to sign your commits for the Developpers Certificate of Originality (DCO) -- every commit has to be signed, for a small change like this, it wouldn't hurt to rebase into a single commit.

@nishihere19
Copy link
Author

Will surely implement the changes as you suggested. Thank you.

@jeromesimeon
Copy link
Member

@nishihere19 I may have spoke a bit too fast. I'm looking at the netlify build and seeing some errors.
Screenshot 2021-04-04 at 4 58 01 PM

I was able to reproduce that locally too. I'm wondering if the fixed script might work on Windows (keeping most of your changes but resolving to node_modules/.bin executables:

#!/usr/bin/env node

'use strict';

/* eslint-env es6 */

const shell = require('shelljs');

shell.rm('-rf', 'demo');
shell.mkdir('demo');

var path = require('path');
var demo = path.resolve('./demo');

var demodata = path.resolve('./scripts/demodata.js');

shell.exec(`node ${demodata} > ./lib/sample.json`);
var pug = path.resolve('./node_modules/.bin/pug');
var index = path.resolve('./lib/index.pug');
shell.exec(`node ${pug} lib/index.pug --obj lib/sample.json --out ${demo}`);

var stylus = path.resolve('./node_modules/.bin/stylus');
var index_styl = path.resolve('./lib/index.styl');

shell.exec(`node ${stylus} -u autoprefixer-stylus \
< ${index_styl} \
> ./demo/index.css`);

var templateMarkCSS_lib = path.resolve('./lib/templatemark.css');
var templateMarkCSS_demo = path.resolve('./demo/templatemark.css');
shell.cp(templateMarkCSS_lib, templateMarkCSS_demo);

shell.rm('-rf', "lib/sample.json");
index=path.resolve('./node_modules/.bin/browserify');

shell.exec(`${index}  lib/index.js \
> demo/index.js --standalone demo/index.html`);

@nishihere19
Copy link
Author

nishihere19 commented Apr 5, 2021

On using node_modules/.bin executables we shall encounter the following errors in Windows.
build_dingus js - Projects - Visual Studio Code 05-04-2021 11_49_57 (2)

Also, while writing the script, the aim was to resolve path issues in Windows. After resolving the above errors, I think we might get path errors again as the path given in .bin scripts resolves to markdown-transform/node_modules rather than markdown-transform/packages/dingus/node_modules.

Are we encountering errors in netlify build after running npm install inside packages/dingus directory as well?

@nishihere19
Copy link
Author

Any updates on this? Any suggestions on how the script can be improved so that it works on Windows without hindering the existing setup for Linux would be great.

@jeromesimeon
Copy link
Member

Any updates on this? Any suggestions on how the script can be improved so that it works on Windows without hindering the existing setup for Linux would be great.

hi @nishihere19 Thanks for pinging, sorry I've been pretty busy lately. I think at this point my recommendation would be to go back to your original PR! Unless we find a better way to investigate why the root ./node_modules doesn't get properly create when installing on Windows.

For the Dingus it's probably okay to have a separate windows build for now.

If you revert to your initial scripts, leaving the current ones in place I think this will unblock this PR.

@nishihere19
Copy link
Author

Upon deleting the node modules and rebuilding the project, I was able to reproduce the error you showed in the screenshots. The possible solution I found was to replace the postinstall script in the package.json for markdown transform from
npm run models:get && lerna bootstrap to npm run models:get && lerna bootstrap && cd packages/dingus && npm install If this change is fine, npm run dingus will work on windows and Linux without producing any errors with the replaced script and we need not have a separate command for windows.
Since the node modules were not installed for dingus, stylus and pug-cli were missing, therefore the script was producing an error.

@jeromesimeon
Copy link
Member

Upon deleting the node modules and rebuilding the project, I was able to reproduce the error you showed in the screenshots. The possible solution I found was to replace the postinstall script in the package.json for markdown transform from
npm run models:get && lerna bootstrap to npm run models:get && lerna bootstrap && cd packages/dingus && npm install If this change is fine, npm run dingus will work on windows and Linux without producing any errors with the replaced script and we need not have a separate command for windows.
Since the node modules were not installed for dingus, stylus and pug-cli were missing, therefore the script was producing an error.

hm... maybe I would want to try this in netlify but if it works, I don't see why not!

Signed-off-by: Nishi Agrawal <[email protected]>
@nishihere19
Copy link
Author

The checks are failing continuously due to the error - cb() never called! I have cleaned the cache and updated npm as well. The command npm ci works fine in my local machine(Windows 10 and ubuntu 20.04) but fails in the checks. I will read more on this.

@dselman dselman closed this May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error while running dingus locally in Windows
5 participants