optional param to prevent function overwriting

[Stuonts]

No description provided.

[sbc100]

I think it might be good to add a test.

What is more, I think we have a test the deliberately overrides some system library, so I'm surprised that that test is not now failing.

[Stuonts]

I think it might be good to add a test.

What is more, I think we have a test the deliberately overrides some system library, so I'm surprised that that test is not now failing.

Test case have been added. Previous tests with rewriting don't fail because of optional flag by default in mergeInto (still rewrites functions by default)

[sbc100]

@juj are you OK with this being an error? Or would you rather see it be warning? I think you are the only user of the symbol overriding that I know of.

[sbc100]

It looks like some of the test failures are because you need to merge (or rebase) with origin/main.

[Stuonts]

@juj are you OK with this being an error? Or would you rather see it be warning? I think you are the only user of the symbol overriding that I know of.

It's better to have error rather than warning for us because:

1. we have large project that can have immense console output, also we use build system that can hide console output

2. basically, we have a lot of libraries and we add .js files associated with particular libraries. All end targets using libraries include .js files in the end and it creates risk of functions redefinitions (it could be mitigated by function names mangling, but we want something more reliable)

[sbc100]

@juj are you OK with this being an error? Or would you rather see it be warning? I think you are the only user of the symbol overriding that I know of.

It's better to have error rather than warning for us because:

1. we have large project that can have immense console output, also we use build system that can hide console output
2. basically, we have a lot of libraries and we add .js files associated with particular libraries. All end targets using libraries include .js files in the end and it creates risk of functions redefinitions (it could be mitigated by function names mangling, but we want something more reliable)

Its possible we could add it as a warning and have -Werror honored in this context which would give the us flexibiity.

On the other hand I agree it would be simpler if we could just stick to and error (and force library authors to opt in with allowOverride)... It really depends how easy it will be for @juj to add allowOverride=true everywhere he needs it?

[Stuonts]

Another suggestion - to add function
mergeIntoNoOverride, so function won't have flags, it might be simpler to maintain for emscripten users

[juj]

@juj are you OK with this being an error? Or would you rather see it be warning? I think you are the only user of the symbol overriding that I know of.

On the other hand I agree it would be simpler if we could just stick to and error (and force library authors to opt in with allowOverride)... It really depends how easy it will be for @juj to add allowOverride=true everywhere he needs it?

Thanks for looking out and accommodating.. it does sound ok to me to make this an error, we should be able to adjust our libraries, and with a clear error message like it is now, it does guide users to what kind of modification they need to do to their code if the override is intentional.

One thing that comes to mind is it seems that now the filename that causes the error is not printed? That would be great to see. Although don't want to rope extra work from @Stuonts if we don't have that facility available now. (at a quick glance to utility.js and compiler.js I'm not sure if we do)

Another way might be to make it a warning and enable the -Werror=override-js-function style of diagnostics directives. Although either way is good to me - happy to go with a path that causes less friction to contributing here.

[Stuonts]

@juj
can try creating separate PR with error displaying - from what file mergeInto causing errors is called

[Stuonts]

@sbc100 , @kripken , @juj
can you please review?

[Stuonts]

@sbc100
thanks for review
seems auto merge failed