search

imodeljs / desktop-starter

An iTwin.js Desktop starter app
https://itwinjs.org
MIT License
21 stars 12 forks source link

Upgrade to 2.13.0 #47

Closed kabentley closed 3 years ago

kabentley commented 3 years ago

update to use new ElectronHost/ElectronApp apis

kabentley commented 3 years ago

@calebmshafer i don't understand the lint errors

williamkbentley commented 3 years ago

@calebmshafer i don't understand the lint errors

I agree for the "rule not found" ones, but this one seems more direct:

\desktop-starter\src\frontend\components\AppComponent.tsx 385:26 error 'RemoteBriefcaseConnection' is deprecated. use BriefcaseConnection with an IpcApp deprecation/deprecation

kabentley commented 3 years ago

@calebmshafer i don't understand the lint errors

I agree for the "rule not found" ones, but this one seems more direct:

\desktop-starter\src\frontend\components\AppComponent.tsx 385:26 error 'RemoteBriefcaseConnection' is deprecated. use BriefcaseConnection with an IpcApp deprecation/deprecation

yes, this app should be reworked to use either CheckpointConnection (if it is meant to only ever open checkpoints) or BriefcaseConnection if it is meant to open/edit Briefcases. For now, we can add ignore to this warning.

kabentley commented 3 years ago

@calebmshafer i don't understand the lint errors

I agree for the "rule not found" ones, but this one seems more direct: \desktop-starter\src\frontend\components\AppComponent.tsx 385:26 error 'RemoteBriefcaseConnection' is deprecated. use BriefcaseConnection with an IpcApp deprecation/deprecation

yes, this app should be reworked to use either CheckpointConnection (if it is meant to only ever open checkpoints) or BriefcaseConnection if it is meant to open/edit Briefcases. For now, we can add ignore to this warning.

is @kckst8 working on this app?

calebmshafer commented 3 years ago

@calebmshafer i don't understand the lint errors

I agree for the "rule not found" ones, but this one seems more direct: \desktop-starter\src\frontend\components\AppComponent.tsx 385:26 error 'RemoteBriefcaseConnection' is deprecated. use BriefcaseConnection with an IpcApp deprecation/deprecation

yes, this app should be reworked to use either CheckpointConnection (if it is meant to only ever open checkpoints) or BriefcaseConnection if it is meant to open/edit Briefcases. For now, we can add ignore to this warning.

is @kckst8 working on this app?

@kabentley he is not currently. Any work done so far has been in the other repo that we have yet to make public. Once 2.13 is released, we may be in a state where we can finally make that repo public. However, I'll need to talk with @kckst8 since there are probably changes he needs to make to the iTwin Viewer reacting to the latest changes.

calebmshafer commented 3 years ago

@calebmshafer i don't understand the lint errors

@kabentley I'll take a look.

calebmshafer commented 3 years ago

@calebmshafer i don't understand the lint errors

@kabentley I'll take a look.

@kabentley just pushed a fix for it. Apparently you need to have both @bentley/eslint-plugin and @typescript-eslint/eslint-plugin installed directly for some of the rules to work. Not quite sure why that's necessary, hopefully @paulius-valiunas can shed some light on it.

kabentley commented 3 years ago

just pushed a fix for it. Apparently you need to have both @bentley/eslint-plugin and @typescript-eslint/eslint-plugin installed directly for some of the rules to work. Not quite sure why that's necessary, hopefully @paulius-valiunas can shed some light on it.

@calebmshafer it now says:

Line 385:5: Definition for rule 'deprecation/deprecation' was not found deprecation/deprecation

any ideas?

paulius-valiunas commented 3 years ago

The lint error is just webpack being weird. If you run npm run lint separately, it won't show any errors.

For some reason webpack insists on using its own built in eslint config which doesn't contain deprecation or any other plugins that we use. There is a way to make it use any config you provide, but I had too many problems with that as it doesn't seem to load things correctly when a config from an external package is used. The workaround we have in the imodeljs monorepo is to provide it a blank config that only loads the plugins but no rules, just so that it doesn't throw these "rule not found" errors on every eslint-disable comment. As a pleasant side effect, it makes the build faster by not running eslint twice. I just pushed it to this branch.

kabentley commented 3 years ago

@calebmshafer @wgoehrig now webpack is crashing after 11 minutes on my box.

desktop-starter@1.0.5 build:frontend D:\temp\desktop-starter cross-env EXTEND_ESLINT=true react-scripts build

Creating an optimized production build... (node:9628) ExperimentalWarning: Package name self resolution is an experimental feature. This feature could change at any time

<--- Last few GCs --->

[9628:00000256F56DCA30] 158529 ms: Mark-sweep 2042.8 (2078.4) -> 2039.5 (2059.7) MB, 164.2 / 0.1 ms (+ 568.5 ms in 119 steps since start of marking, biggest step 11.5 ms, walltime since start of marking 773 ms) (average mu = 0.103, current mu = 0.052)

<--- JS stacktrace --->

==== JS stack trace =========================================

0: ExitFrame [pc: 00007FF74AFB4FAD]
1: StubFrame [pc: 00007FF74AFB5EDD]

Security context: 0x0312205408d1 2: / anonymous /(aka / anonymous /) [0000019761F0C1D1] [D:\temp\desktop-starter\node_modules\webpack-sources\lib\applySourceMap.js:~58] [pc=000003744712A8EA](this=0x012b60f804b1 ,0x0099ee44f751 <String[8]: default:>,0x004b5cc10df9 ) 3: SourceNode_walk [000002208EF...

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory

Writing Node.js report to file: report.20210217.134558.9628.0.001.json Node.js report completed 1: 00007FF74A3B380F napi_wrap+128063 2: 00007FF74A3529B6 public: bool cdecl v8::base::CPU::has_sse(void)const ptr64+35142 3: 00007FF74A353676 public: bool cdecl v8::base::CPU::has_sse(void)const ptr64+38406 4: 00007FF74AB6A11E private: void cdecl v8::Isolate::ReportExternalAllocationLimitReached(void) ptr64+94 5: 00007FF74AB521F1 public: class v8::SharedArrayBuffer::Contents cdecl v8::SharedArrayBuffer::Externalize(void) ptr64+833 6: 00007FF74AA1E74C public: static void cdecl v8::internal::Heap::EphemeronKeyWriteBarrierFromCode(unsigned int64,unsigned int64,class v8::internal::Isolate * ptr64)+1436 7: 00007FF74AA299A0 public: void cdecl v8::internal::Heap::ProtectUnprotectedMemoryChunks(void) ptr64+1312 8: 00007FF74AA264C4 public: static bool cdecl v8::internal::Heap::PageFlagsAreConsistent(class v8::internal::HeapObject)+3204 9: 00007FF74AA1BCE3 public: bool cdecl v8::internal::Heap::CollectGarbage(enum v8::internal::AllocationSpace,enum v8::internal::GarbageCollectionReason,enum v8::GCCallbackFlags) ptr64+1283 10: 00007FF74AA1A354 public: void cdecl v8::internal::Heap::AddRetainedMap(class v8::internal::Handle) ptr64+2452 11: 00007FF74AA3B53D public: class v8::internal::Handle cdecl v8::internal::Factory::NewFillerObject(int,bool,enum v8::internal::AllocationType,enum v8::internal::AllocationOrigin) ptr64+61 12: 00007FF74A7A0E21 public: class v8::internal::interpreter::JumpTableTargetOffsets::iterator & ptr64 cdecl v8::internal::interpreter::JumpTableTargetOffsets::iterator::operator=(class v8::internal::interpreter::JumpTableTargetOffsets::iterator && ptr64) ptr64+1665 13: 00007FF74AFB4FAD public: virtual bool cdecl v8::internal::SetupIsolateDelegate::SetupHeap(class v8::internal::Heap ptr64) ptr64+546637 14: 00007FF74AFB5EDD public: virtual bool __cdecl v8::internal::SetupIsolateDelegate::SetupHeap(class v8::internal::Heap ptr64) ptr64+550525 15: 000003744712A8EA

calebmshafer commented 3 years ago

@kabentley @williamkbentley this should be working now. Let me know if you run into any other issues.

I no longer get any problems when building and was able to successfully run the app.

kabentley commented 3 years ago

I no longer get any problems when building and was able to successfully run the app.

But the build is still failing...

calebmshafer commented 3 years ago

I no longer get any problems when building and was able to successfully run the app.

But the build is still failing...

@kabentley let me try it out on a lower speced machine to see if I can reproduce.

williamkbentley commented 3 years ago

@kabentley Can you review? I implemented CheckpointConnection and made some changes to npm start script

calebmshafer commented 3 years ago

@williamkbentley for some reason I get a blank screen on initial startup. I wonder if there's something strange going on with pulling up a previously cached iModel. Once I refresh though it starts working again. Possibly something that existed prior to this PR.

kabentley commented 3 years ago

@kabentley Can you review? I implemented CheckpointConnection and made some changes to npm start script

No, this isn't right. This is what i've been trying to get @kckst8 or someone to fix. Desktop apps should acquire a briefcase and then use BriefcaseConnection (CheckpointConnection is only for web apps connected to remote backends - that's what openRemote means). There needs to be some way to request to synch your local briefcase to newer versions. Let me add some code that shows what i mean.

kabentley commented 3 years ago

@williamkbentley @calebmshafer @kckst8

I roughed in how i think this should work. I need to work on the backend to support pullAndMerge on readonly briefcases, but you should at least be able to get this started now.

kabentley commented 3 years ago

@calebmshafer why does the linter here work differently than in the monorepo? Also, the build process seems to hide all errors unless you run the leaf tasks

calebmshafer commented 3 years ago

@kabentley @williamkbentley @wgoehrig everyone mind re-reviewing? This PR should be good to go now.

@kabentley I had to switch back to use function properties in order for the this.setState to work correctly. Not entirely sure why, someone with more React experience would need to fill in the details...

Either way, hoping that we can replace this whole AppComponent with a new functional component that uses the iTwin Viewer shortly anyway.

williamkbentley commented 3 years ago

@kabentley @williamkbentley @wgoehrig everyone mind re-reviewing? This PR should be good to go now.

@kabentley I had to switch back to use function properties in order for the this.setState to work correctly. Not entirely sure why, someone with more React experience would need to fill in the details...

Either way, hoping that we can replace this whole AppComponent with a new functional component that uses the iTwin Viewer shortly anyway.

There is still a TODO in AppComponent.

kabentley commented 3 years ago

@calebmshafer i made a different fix, that i think is better. But, hopefully we replace the whole thing soon.

calebmshafer commented 3 years ago

@calebmshafer i made a different fix, that i think is better. But, hopefully we replace the whole thing soon.

@kabentley sounds good. You okay with me reverting this PR to 2.13? I'd like to get it merged once we release 2.13 (hopefully today).

kabentley commented 3 years ago

@kabentley sounds good. You okay with me reverting this PR to 2.13? I'd like to get it merged once we release 2.13 (hopefully today).

Oh sorry, I was going to try to use a change i made recently in 2.14 to eliminate SyncMode, but it's not there yet anyway. 2.13 is fine.

calebmshafer commented 3 years ago

There is still a TODO in AppComponent.

@williamkbentley thanks, removed them both for now. They will be addressed in a separate PR.

calebmshafer commented 3 years ago

@williamkbentley mind taking a look and approving?

  • © Githubissues.
  • Githubissues is a development platform for aggregating issues.