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

Chore: Add npm script for new core developers #7539

Merged
merged 5 commits into from Oct 16, 2023

Conversation

linonetwo
Copy link
Contributor

@linonetwo linonetwo commented Jun 14, 2023

When I first try to develop tw core, it was hard to find how to dev and how to test.

Other opensource projects use npm script as a "self-explained" way to provide dev methods. You don't need to look at ./bin/ folder, you just need to look at VSCode's

截屏2023-06-14 13 07 43

And click "test" button to run a test, this is very standardized. So I also add it to the tw for potential new developers (and me).

@vercel
Copy link

vercel bot commented Jun 14, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
tiddlywiki5 ✅ Ready (Inspect) Visit Preview Sep 15, 2023 0:15am

@pmario
Copy link
Contributor

pmario commented Jun 14, 2023

I think it should not be needed to run any command from the .bin directory at all. It's not necessary and many of them are outdated. Only Jeremy knows what can be used and what not.

If we would like to use package.json scripts we need to make sure that they work with every OS a contributor may use.

TW works well with node tiddlywiki .... commands if the main dependencies are installed in the right way. IMO for the basics: https://tiddlywiki.com/#GettingStarted%20-%20Node.js should work. If it does not it needs to be fixed.

So Node.js, npm, git and TiddlyWiki should be the only dependencies that are needed. We could even discuss about git.

They should also work with different development setups. So every developer may have their own directory structure.

@pmario
Copy link
Contributor

pmario commented Jun 14, 2023

I absolutely support the idea to have package.json scripts for all the needed functions to work with and test TiddlyWiki, if it's done in a generic way.

@Jermolene
Copy link
Owner

Thanks @linonetwo I would very much like to have some sensible defaults in "scripts" in package.json. I'm open to any changes that make it easier for developers coming to TiddlyWiki for the first time.

I agree with @pmario that it would be best to ignore the scripts in ./bin. Many of those scripts are quite old, and were originally added to make things more convenient for me personally. As a separate discussion, I'd welcome a cleanup of those scripts.

So, we'd be left with commands like this:

     "dev": "tiddlywiki ./editions/tw5.com-server --listen",

Can we parameterise those commands so that one could optionally pass the edition path? For example:

npm run dev ./editions/geospatialdemo

@pmario
Copy link
Contributor

pmario commented Jun 14, 2023

@Jermolene .. IMO it's important to know, which bin/xx.sh files you still use to deploy a system at the moment.

For the first run we should keep them and start to remove the old ones.

BUT IMO we need to take care that building TWclassic still works. ... @YakovL Do you still need some stuff from the TW5 repo, to build TWc??

@Jermolene
Copy link
Owner

@Jermolene .. IMO it's important to know, which bin/xx.sh files you still use to deploy a system at the moment.

I think that should be a separate discussion

@linonetwo
Copy link
Contributor Author

linonetwo commented Jun 14, 2023

Yes, you can use -- to pass params to it, but I tried

"dev": "node ./tiddlywiki.js --listen",

with result

% npm run dev -- ./editions/tw5.com-server

> tiddlywiki@5.3.0-prerelease dev
> node ./tiddlywiki.js --listen ./editions/tw5.com-server

Error: malformed named parameter: './editions/tw5.com-server'

If you figure out what happened, hope you can add new scripts dev:tw5.com-server like

    "dev": "node ./tiddlywiki.js --listen",
    "dev:tw5.com-server": "node ./tiddlywiki.js ./editions/tw5.com-server --listen",

To leave a default script (dev:tw5.com-server) for me to click-button-to-run.

@linonetwo
Copy link
Contributor Author

linonetwo commented Jun 14, 2023

I mostly use buttons to run npm script, so there is no an input to add param. So a default version without additional param is needed.

I also want to introducegoogle/zx to write scripts like https://github.com/tiddly-gittly/slate-write/tree/master/scripts , but that will need a rewrite of all sh, I don't want to do that...

@YakovL
Copy link

YakovL commented Jun 14, 2023

@pmario yes, currently TWC is built using the TW5 cli, although I'd like to change the building tool at some point for several reasons (including autotests, imports and IDE support etc). But as package.json has the TW5 version specified, I don't think that any update will hurt

@pmario
Copy link
Contributor

pmario commented Jun 14, 2023

I personally would like to be able to have a basic development experience without the need to ever run npm install

If it is needed for "progressively enhanced" commands it may be fine.

But for building empty.html, tiddlywiki5.html and running all tests git clone https://github.com/Jermolene/TiddlyWiki5.git should be enough.

@Jermolene
Copy link
Owner

Thanks @linonetwo @pmario

I personally would like to be able to have a basic development experience without the need to ever run npm install

I strongly agree, I'd hope we could avoid it. Presumably that would mean we need to guard against adding dependencies that require npm install.

It's not practical to have separate scripts for building each edition. I think that means we need to view these script commands as serving several needs:

  • reasonable defaults for evaluating TiddlyWiki via the default npm commands like start, stop, test, and version
  • examples to help people learn how to construct their own command sequences
  • maybe there are some of the standard npm lifecycle scripts that would be useful to define, but I couldn't see anything obvious

With that frame of reference, we don't need to complexify things with parameters.

We don't currently use npm scripts for CI but perhaps it might help with onboarding and reuse (I'm thinking of npm run build triggering ./bin/build-site.sh, or maybe it should be npm build?).

@linonetwo
Copy link
Contributor Author

linonetwo commented Jun 15, 2023

npm build is same as npm run build.

It's not practical to have separate scripts for building each edition.

npm script is a standard dev doc for developers. So we can only add a command to start an edition, "command to start other editions" can be constructed by copying the script from package.json to the terminal and modifying it.

standard npm lifecycle scripts that would be useful to define

I rarely use them too.

@saqimtiaz
Copy link
Contributor

It has been a while since I worked with this but here is a very quick example of providing the edition as a commandline argument:

"dev": "tiddlywiki $npm_config_twedition --listen"

Run with:
npm run dev --twedition=./editions/tw5.com/

Commandline arguments are available as variables prefixed with $npm_config in the scripts, and as environmental variables int the scripts executed. We could also write a short wrapper script that assigns a default edition if one is not provided and then runs tiddlywiki.

@linonetwo
Copy link
Contributor Author

linonetwo commented Jun 15, 2023

Interesting, haven't heard about this before. Better if it can provide a default value, so I can click button to start dev. I don't like using terminal and cli. But you said this requires a .sh file, and windows can't run this.

This may be a separate script like "dev:edition": xxx, using : to represent sub script is common.

@pmario
Copy link
Contributor

pmario commented Jun 15, 2023

This may be a separate script like "dev:edition": xxx, using : to represent sub script is common.

I think we can figure out, what is convenient to use after we did some more testing with different platforms.

@linonetwo
Copy link
Contributor Author

I need this to be merged to add more scripts, like the inspector script for #7152 (comment)

@pmario
Copy link
Contributor

pmario commented Jun 16, 2023

As I wrote test.sh cannot be executed on windows systems. About 60% of the desktop world uses windows. We should not ignore this fact.

So as it is I'm against merging this PR

@linonetwo
Copy link
Contributor Author

You mean test script? Let me see how I can change that...

@linonetwo
Copy link
Contributor Author

linonetwo commented Jun 16, 2023

Fixed!

截屏2023-06-16 16 49 45

And that echo exist in windows' shell, so I think it's fine.

Now that npm script is identical to the original shell script, you can consider deleting them in another PR.

package.json Outdated
"dev": "node ./tiddlywiki.js ./editions/tw5.com-server --listen",
"test": "node ./tiddlywiki.js ./editions/test --verbose --version --rendertiddler $:/core/save/all test.html text/plain --test && echo To run the tests in a browser, open \"editions/test/output/test.html\"",
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this should do the same thing node ./tiddlywiki.js ./editions/test --verbose --version --build index
except the echo.
--build index is part of the test-edition tiddlywiki.info configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really? I can modify this tomorrow. I just copied it from the .sh file

@linonetwo
Copy link
Contributor Author

linonetwo commented Sep 15, 2023

Bump, I really need this, I forget the command to run today again, and have to come here to copy node ./tiddlywiki.js ./editions/tw5.com-server --listen from this PR...

@pmario @Jermolene

@linonetwo
Copy link
Contributor Author

linonetwo commented Sep 15, 2023

Can we delete tw5.com-server / tw5.com-docs ? They are confusing. I try to update doc today, and found I was updating tw5.com edition, but I'm serving tw5.com-server.


Oh, seems it is just a pointer... Never mind.

@linonetwo
Copy link
Contributor Author

Have to visit this PR to copy the command again...

@Jermolene
Copy link
Owner

Thanks @linonetwo

@Jermolene Jermolene merged commit 9773dff into Jermolene:master Oct 16, 2023
4 checks passed
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

5 participants