Skip to content
This repository was archived by the owner on Jul 10, 2025. It is now read-only.

Conversation

coder11
Copy link
Contributor

@coder11 coder11 commented Dec 8, 2022

  1. Move marine-related part into FJS repo (DXJ184)
  2. Move towards component-oriented architecture (DXJ183)
  3. Different JS Client distros for node.js and web (DXJ185)

@linear
Copy link

linear bot commented Dec 8, 2022

DXJ-135 Move relevant logic from MarineJS into FluenceJS

move as much logic as possible out of Marine repo into FJS (as a project of monorepo). I mean leaving only auto-generated code and the bare minimum interface to call to Marine directly. Also leaving only vital tests which check the Marine runtime code.
From architectural POW it means

  1. clear separation of concerns.
  2. ability to use MarineJS directly (e.g reuse a service for small command-line utility)

From practical POW it means

  1. Much simpler and more relevant test cases
  2. Simpler CI pipeline for Marine repo
  3. Easier to make changes to background worker boilerplate; not touching code from Marine repo

@CLAassistant
Copy link

CLAassistant commented Dec 8, 2022

CLA assistant check
All committers have signed the CLA.

@coder11 coder11 force-pushed the DXJ-135-move-marine branch from f0126fd to 4ec2720 Compare December 8, 2022 12:34
@coder11 coder11 force-pushed the DXJ-135-move-marine branch from bd5c565 to e1a1907 Compare December 14, 2022 21:07
@coder11 coder11 requested a review from shamsartem January 4, 2023 07:15
Copy link
Contributor

@shamsartem shamsartem left a comment

Choose a reason for hiding this comment

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

Please reply to comments and maybe add some changes

server = new WebpackDevServer(devServerOptions, compiler);
await server.start();
// wait for webpack to load
await new Promise((resolve) => setTimeout(resolve, 1000));
Copy link
Contributor

Choose a reason for hiding this comment

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

there must be a better way. It's just not cool. What happens if you don't do this? If there is some error maybe instead of timeout it's better to have an interval that is constantly checking if there is an error and when it dissapears - resolve the promise. But I hope there is some property or something that can help make sure it's loaded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 75 to 78
const res = await page.evaluate(async () => {
// @ts-ignore
return await window.MAIN();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

the equivalent is await page.eveluate(() => window.MAIN())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


export const setLogLevel = (level: LogLevelDesc) => {
log.setLevel(level);
};

log.setDefaultLevel('WARN');

const defaultPeer = new FluencePeer();
let pkg: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain this package purpose in README. If it is temporary compatibility thing - I don't really understand how will it work with require in browser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a temporary compatibility thing. It didn't work though, so I've changed this logic a little bit

@coder11 coder11 requested a review from shamsartem January 9, 2023 07:09
let server;
const port = 8080;

jest.setTimeout(10000);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is that doing? I just don't know

Copy link
Contributor

@shamsartem shamsartem left a comment

Choose a reason for hiding this comment

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

There is a lot of unfinished stuff, most of which is moved to separate tasks. Let's just move on from this 220 file PR and I hope it will all be solved in those separate tasks. Thanks!

@coder11 coder11 merged commit 267ebb6 into before-denver Jan 9, 2023
@coder11 coder11 deleted the DXJ-135-move-marine branch January 9, 2023 12:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants