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

js -d:nodejs now supports osenv: getEnv, putEnv, envPairs, delEnv, existsEnv (v2) #15826

Merged
merged 4 commits into from
Nov 12, 2020

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Nov 2, 2020

alternative approach to #15475, aimed at minimizing impact on existing code to reduce complexity, so that nodejs can be supported on a best effort basis without adding undue burden on maintenance.

As you can see here https://github.com/nim-lang/Nim/pull/15826/files?diff=split&w=1 (hiding whitespace in the diff), the diff essentially amounts to adding a a single when defined(nodejs): X else: Y and re-indenting the existing Y

  • getEnv + friends now work with -d:nodejs
  • getEnv + friends still works in VM (with js or js+nodejs or other backend)

see relevant discussion in previous PR, in particular rationale eg #15475 (comment)

future work

@timotheecour timotheecour marked this pull request as ready for review November 2, 2020 18:30
@timotheecour timotheecour changed the title js -d:nodejs now supports osenv: getEnv, putEnv, envPairs, delEnv, existsEnv js -d:nodejs now supports osenv: getEnv, putEnv, envPairs, delEnv, existsEnv (v2) Nov 2, 2020
@timotheecour timotheecour requested a review from Araq November 2, 2020 19:59
@nc-x
Copy link
Contributor

nc-x commented Nov 7, 2020

Before merging this, please first discuss a policy regarding future maintenance and the criteria for supporting/not supporting other nodejs functions.

supported on a best effort basis

IMO supported is pretty much the opposite of best effort basis in software development

Most other languages have something like supported tiers/platforms on their website, IMO we should have it too (Apologies, if it is already present somewhere, I could not find it) -
https://ziglang.org/#Support-Table
https://doc.rust-lang.org/nightly/rustc/platform-support.html
https://haxe.org/documentation/introduction/compiler-targets.html
image

@timotheecour
Copy link
Member Author

timotheecour commented Nov 7, 2020

please first discuss a policy regarding future maintenance and the criteria for supporting/not supporting other nodejs functions.

nodejs would be a Tier 2: mostly maintained by individuals, tested in testament as part of CI, with the explicit 2-fold goals:

  • avoiding software silos (eg: software not written with nodejs in mind should ideally work for nodejs, without requiring patching code)
  • PR's that improve nodejs support are welcome so long they don't add undue burden on code maintenance for the officially supported backends

Most other languages have something like supported tiers/platforms on their website, IMO we should have it too

good idea

@Araq
Copy link
Member

Araq commented Nov 9, 2020

Tiers only help a little bit, in practice we would still get bug reports and PRs for the "Tier 2" targets. You can only argue that the benefits are worth the costs for -d:nodejs. And I doubt it.

@nc-x
Copy link
Contributor

nc-x commented Nov 9, 2020

Tiers only help a little bit, in practice we would still get bug reports and PRs for the "Tier 2" targets.

Well, that's the thing. The issues and pr's will get opened whether you apply this tiering mechanism or not. So there are two options -

  1. Close all issues and prs related to nodejs.
  2. Mark all such issues and pr's with label 'Tier 2 - Community Supported' and/or assign Timothee to them 😄

Personally I am fine with both. I do not use nodejs, and I would prefer the core team to focus on important features and bug fixes.

Edit:
In any case, if this PR is accepted, then IMO we need to have these tiers defined. Tiers might not mean much for the core team, but for any person who might be evaluating nim for a particular use case, these tiers would mean a lot.

@juancarlospaco
Copy link
Collaborator

jsosenv.nim on Fusion.

@timotheecour
Copy link
Member Author

timotheecour commented Nov 10, 2020

jsosenv.nim on Fusion.

this would entirely defeat the purpose, it would make any code containing import os incompatible with nodejs, ie a software silo; you'd have to end up duplicating existing functionality (not just in os.nim but in modules that import os.nim) for no benefit.

The costs involved are being blown out of proportion; accomodating nodejs can be written (as in this PR) with minimal interaction. And yes, I've written quite a few PR's for os.nim.

One of the main points of os.nim is to abstract away platform/backend differences so that other libraries written on top of it work across the board without having to deal with platform specific logic to a large extent. That's the real benefit.

@Araq Araq merged commit cc88291 into nim-lang:devel Nov 12, 2020
@timotheecour
Copy link
Member Author

Thanks for merging this.

@timotheecour timotheecour deleted the pr_nodejs_osenv_2 branch November 12, 2020 17:14
PMunch pushed a commit to PMunch/Nim that referenced this pull request Jan 6, 2021
…Env`, `existsEnv` (v2) (nim-lang#15826)

* js -d:nodejs now supports osenv: `getEnv`, `putEnv`, `envPairs`, `delEnv`, `existsEnv`

* refactor to osenv

* fix for js (without -d:nodejs) + VM

Co-authored-by: Andreas Rumpf <[email protected]>
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
…Env`, `existsEnv` (v2) (nim-lang#15826)

* js -d:nodejs now supports osenv: `getEnv`, `putEnv`, `envPairs`, `delEnv`, `existsEnv`

* refactor to osenv

* fix for js (without -d:nodejs) + VM

Co-authored-by: Andreas Rumpf <[email protected]>
@timotheecour timotheecour added TODO: followup needed remove tag once fixed or tracked elsewhere and removed TODO: followup needed remove tag once fixed or tracked elsewhere labels Mar 2, 2021
irdassis pushed a commit to irdassis/Nim that referenced this pull request Mar 16, 2021
…Env`, `existsEnv` (v2) (nim-lang#15826)

* js -d:nodejs now supports osenv: `getEnv`, `putEnv`, `envPairs`, `delEnv`, `existsEnv`

* refactor to osenv

* fix for js (without -d:nodejs) + VM

Co-authored-by: Andreas Rumpf <[email protected]>
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
…Env`, `existsEnv` (v2) (nim-lang#15826)

* js -d:nodejs now supports osenv: `getEnv`, `putEnv`, `envPairs`, `delEnv`, `existsEnv`

* refactor to osenv

* fix for js (without -d:nodejs) + VM

Co-authored-by: Andreas Rumpf <[email protected]>
@metagn metagn removed the TODO: followup needed remove tag once fixed or tracked elsewhere label Jan 4, 2024
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.

5 participants