Adapt theia build to automate required PhosphorJS change

[lucas-koehler]

PhosphorJS does not allow attaching a widget into another window. As the project is already archived and we do not want to fork it for (because Theia might abandon PhosphorJS in the future), we need to adapt it locally. Luckily, only one specific line needs to be removed from the PhophorJS code.

This is currently done manually via the following command (on Linux):

sed -e '/Host is not attached/s/^/\/\//g' -i node_modules/@phosphor/widgets/lib/widget.js

This removes and exception being thrown when the widget is attached in another window. However, sed is bash tool and users should not need to execute an 'arbitrary' command line command.

Ideally, the webpack config templates for Theia's browser and electron apps are adapted so that this line is automatically removed during the build. Make sure that this works on all OS.

[thegecko]

As the project is already archived and we do not want to fork it for (because Theia might abandon PhosphorJS in the future)

TBF, forking this may be the easiest way forward, especially as Phosphor won't change. Managing a patch on a fork/branch without worrying about upstream changes feels more maintainable than hacking out source code at build time.

[lucas-koehler]

@thegecko Thanks for the input. We initially didn't want to fork because of the overhead of needing to find a place for the fork and publishing it on npm.
As the code is stable, the removal at build time should also be stable.
But if you want to fork and publish the patched version or you have another way to integrate it nicely in the build, feel free to do it this way. I agree that it is the cleaner solution and also allows adding other fixes if they become necessary in the future.
However, I would prefer that it is not going to be required for users of the MVP to check out and build the fork themselves. If that were necessary, I'd prefer the build time 'hack' instead.

What do you think?

[thegecko]

Happy to try the forking approach (perhaps publishing on GitHub rather than npm) if time permits.

[thegecko]

I've had a quick look at this and got a fork building in Github Actions:

https://github.com/thegecko/phosphor/actions

I see you need to remove this error?

Approaches I can think of...

• hack out the line using sed as you suggest. I worry this approach may grow if you need further modifications in future
• hack around the error by redefining document when the widget is attached (I assume the host is attached elsewhere, can we create a temp object which looks like document) here?
• update the source and publish it to GitHub. This approach will need all Theia npm registry requests to go through GitHub with a .npmrc file and GitHub key (see here). Could get messy
• update the source and publish to npm. This needs us to publish under a different name/org, but I worry about this notice on phosphor. Would it be a hard fork?

Thoughts?

[koegel]

IMHO, any fork of Phosphor with additional changes is a hard fork according to the comment on their repo since there is no way to contribute any changes back to IMHO, any fork of Pho, so we are required to change the name of the project (sidetracking: I think SulphurJS would be a good name :) ).

If we really want to enable Theia developers to make changes to Phosphor I think we need to:

• Fork it on Github under a new name (e.g. SulphurJS)
• Open a CQ to be able to move the repo into the Theia org
• Configure a build
• Publish to npm
• Consume from npm

This all is probably quite some work and we are not yet sure if we really want to keep Phosphor on the long run. We are currently investigating what the options are with regards to Phophor ,e.g.:

• Fork
• Replace with other framework
• Replace with custom code / own framework within Theia

Given this and that we currently only require one change to get a MVP of MDI "out of the door" I would opt for the "dirty" build-time hack :). Let me know what you think, @thegecko.

[lucas-koehler]

Just to add to that to have that cleared up:

I see you need to remove this error?

Yes, that is exactly the error we need to remove.

[thegecko]

Suggestion in #31

[lucas-koehler]

Fixed via #32