Add extra variants for suffixes in bsconfig.json

[nkrkv]

trafficstars

• .bs.mjs, .bs.cjs to fill the gap between .bs.js and .mjs
• .res.js, .res.mjs, .res.cjs to assist BS → ReScript rebranding

[cristianoc]

That's a lot of extensions. Are they all really strictly needed to solve the issue discussed in the forum?

[bobzhang]

There is a problem when you have both a.res and a.res.js, the require will pick the former which isn't what you want

[nkrkv]

That's a lot of extensions. Are they all really strictly needed to solve the issue discussed in the forum?

No. Only .bs.mjs and .bs.cjs will solve the problem.

But, we are moving out from the bs abbrev everywhere, aren’t we? I think it’s just a good moment to add three .res.* suffixes as well, so that we’re moving forward preserving backward compatibility.

There is a problem when you have both a.res and a.res.js, the require will pick the former which isn't what you want

I see no problem with specifying a full path. Like require("./SomeResModule.res.js"). Or what do you mean?

[nkrkv]

We could discourage usage of “old” .bs infix by simplifying the error message to

 Bsb_exception.errorf ~loc
          "expect .js|.mjs|.cjs or .res.js|.res.mjs|.res.cjs here"

But all the users of .bs.js will still have everything working.

[Kingdutch]

Is there a reason we're extending the existing string enum? Can't we make it a freeform string field where the only constraint is that it starts with a .? Would other code in the compiler be complicated by having this be unpredictable?

[nkrkv]

Can't we make it a freeform string field where the only constraint is that it starts with a .

Like .; || rm -rf ~/ ? :smile:

Well, it can be ^\.[A-Za-z0-9_-\.]+$ actually validated at the level of bsconfig.json schema.

Wdyt?

[Kingdutch]

I have no real opinion for user input validation here beyond what's absolutely necessary but I don't see the need for anything beyond \..*.

Like .; || rm -rf ~/ ? 😄

Except for the slash there, that's a valid filename. So I don't know why we'd have to be smarter than operating systems and tools like those provided by GNU.

$ touch "Component.; || -rf *"
$ ls
Component.; || -rf *

Well, it can be ^.[A-Za-z0-9_-.]+$ actually validated at the level of bsconfig.json schema.

This misses a whole host of unicode characters that someone may want to use as file extension for a reason we can't fathom yet (similarly to how we previously didn't think of wanting .bs.mjs).

From an actual possible threat perspective (and why I don't think input validation buys us much):

• The value provided in this field of the bsconfig.json shouldn't actually be executed
• The value provided in this field is only ever respected for the bsconfig.json that is at the root of a project (so the values in dependencies are ignored), so even a malicious ReScript package could not exploit this.

If we're worried about someone adding something malicious here and git cloneing that package, running npm install and rescript, then the easier PWN moment would already be in npm install :)

We could even drop the "must start with ." validation because someone may want ReScript.mjs as suffix to distinguish files and deal with tools that don't handle double file extensions 🤷‍♂️

[Kingdutch]

Another thought but maybe out of scope for this PR and to be done in a follow-up:

It may be interesting to allow specifying extensions for different sources and allow sources to have glob expressions. Two example use cases:

1. I want my application to be in ESM format, but the library I'm interoperating with still requires CJS configuration that I also want to be able to write in ReScript (So app/ -> .mjs and config/ -> .cjs)
2. I want to be able to re-use stories from Storybook writting in ReScript so can not use MyStory.stories.res naming because that's not importable. Instead I want to convert all story modules to .stories.mjs and all other modules to .mjs (So src/*Story -> .stories.mjs and src/{!*Story} -> .mjs

That flexibility would also once and for all end any "I need some code to have a specific extension to interop with this new framework" issues.

[bobzhang]

I see no problem with specifying a full path. Like require("./SomeResModule.res.js"). Or what do you mean?

require('./xx.res') does not work while require('./xx.res.js') works, this would cause some surprise.

Let's only add '.bs.mjs' if that already solves your problem

[nkrkv]

If one imports './xx.res' and xx.res is indeed exists, I see no surprise. He asks for a specific file, he gets it.

After all, in the ESM land, the extension is mandatory by design:

A file extension must be provided when using the import keyword to resolve relative or absolute specifiers. Directory indexes (e.g. './startup/index.js') must also be fully specified. This behavior matches how import behaves in browser environments, assuming a typically configured server.

[cknitt]

I just migrated a project to .mjs and now feel the need for a .bs.mjs or .res.mjs, too. 🙂

Let's only add '.bs.mjs' if that already solves your problem

That would also solve the immediate problem for me. But I agree with @Kingdutch and @nkrkv that:

• we should get rid of the remainders of the "BuckleScript" branding
• a freely configurable suffix would be preferable

But as there are divergent opinions on this, maybe it would be best to move ahead with just .bs.mjs (and maybe .bs.cjs?) now in order to get this included in release 10.1 and then continue the discussion for bigger changes in 11.0?

[nkrkv]

But as there are divergent opinions on this, maybe it would be best to move ahead with just .bs.mjs (and maybe .bs.cjs?) now in order to get this included in release 10.1

Makes sense. I’ve updated the PR to remove .res.* support. @cristianoc @bobzhang please, take a look

[cristianoc]

But as there are divergent opinions on this, maybe it would be best to move ahead with just .bs.mjs (and maybe .bs.cjs?) now in order to get this included in release 10.1

Makes sense. I’ve updated the PR to remove .res.* support. @cristianoc @bobzhang please, take a look

That looks good. Would you also update the CHANGELOG.

[nkrkv]

Would you also update the CHANGELOG.

Sure. Should it be under 10.1.0-alpha.2 (already released I guess), or should I create a 10.1.0-alpha.3 section?

[cristianoc]

Would you also update the CHANGELOG.

Sure. Should it be under 10.1.0-alpha.2 (already released I guess), or should I create a 10.1.0-alpha.3 section?

Good question. This is master so it goes on top (11.0). If one wants this to ship with 10.1, a separate PR should go into that branch, which cherry picks this. For this reason, should remember to squash+merge.

[cknitt]

Would definitely like to have this in 10.1! 🙂

@nkrkv Once this is merged to master, would you do a PR to cherry-pick to the 10.1_release branch?

[cristianoc]

Would definitely like to have this in 10.1! 🙂

@nkrkv Once this is merged to master, would you do a PR to cherry-pick to the 10.1_release branch?

In fact 10.1 and master changelogs have diverged a bit so no matter what, a bit of cleanup is required.

[cristianoc]

Thanks. I'll merge then do the clean up and cherry picking.