Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix build error on windows #103

Merged
merged 4 commits into from
Nov 26, 2020
Merged

fix build error on windows #103

merged 4 commits into from
Nov 26, 2020

Conversation

kfrederix
Copy link
Contributor

@kfrederix kfrederix commented Nov 26, 2020

Fixes #102

@codecov-io
Copy link

codecov-io commented Nov 26, 2020

Codecov Report

Merging #103 (0286b06) into master (f6fcd1e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #103   +/-   ##
=======================================
  Coverage   94.26%   94.27%           
=======================================
  Files          41       41           
  Lines        1482     1484    +2     
  Branches      340      340           
=======================================
+ Hits         1397     1399    +2     
  Misses         85       85           
Impacted Files Coverage Δ
src/partialHydration/prepareFindSvelteComponent.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6fcd1e...0286b06. Read the comment docs.

@kfrederix
Copy link
Contributor Author

It seems to break the tests somehow, sorry. I 'll try fix that...

@nickreese
Copy link
Contributor

@kfrederix I think glob expects linux style paths. This has tripped me up before. Are the globs returning empty?

I really need a windows dev env.

@nickreese
Copy link
Contributor

I’m mobile but if the tests are using path.resolve on the mock’d glob that would cause inconsistency with how glob works. Could be a bug in the test.

@kfrederix
Copy link
Contributor Author

kfrederix commented Nov 26, 2020

@nickreese yeah I think you are right. I am currently out of time to spend on this unfortunately. I 'll check back when I have more time in the upcoming days...

@kfrederix
Copy link
Contributor Author

@nickreese tests are passing now on windows as well. I made a few more changes to prepareFindSvelteComponent to make it more robust...

@nickreese
Copy link
Contributor

@kfrederix 👏 Awesome. Looks good. Thank you.

@nickreese nickreese merged commit 1c17a8d into Elderjs:master Nov 26, 2020
@x4080
Copy link

x4080 commented Dec 25, 2020

Hi @kfrederix , I tried the latest (v1.2.4) and still wont run in windows

> Elder.js running on http://localhost:3000
/ [
  TypeError [ERR_INVALID_ARG_TYPE]: The "id" argument must be of type string. Received undefined
      at validateString (internal/validators.js:124:11)
      at Module.require (internal/modules/cjs/loader.js:945:3)
      at require (internal/modules/cjs/helpers.js:88:18)
      at Object.templateComponent (D:\svelteprojects\elderjs-app\node_modules\@elderjs\elderjs\build\utils\svelteComponent.js:20:20)
      at buildPage (D:\svelteprojects\elderjs-app\node_modules\@elderjs\elderjs\build\utils\Page.js:43:40)
      at processTicksAndRejections (internal/process/task_queues.js:93:5)
      at async Object.run (D:\svelteprojects\elderjs-app\node_modules\@elderjs\elderjs\build\hooks\index.js:81:34)
      at async D:\svelteprojects\elderjs-app\node_modules\@elderjs\elderjs\build\utils\prepareRunHook.js:46:44
      at async processHook (D:\svelteprojects\elderjs-app\node_modules\@elderjs\elderjs\build\utils\prepareRunHook.js:41:13)
      at async Array.prepServer (D:\svelteprojects\elderjs-app\node_modules\@elderjs\elderjs\build\utils\prepareServer.js:8:9) {
    code: 'ERR_INVALID_ARG_TYPE'
  }
]

Did it work in your side ?

@kfrederix
Copy link
Contributor Author

kfrederix commented Dec 27, 2020

@x4080 yes it works for me with latest version. But please note: in the elderjs template the version is still locked to 1.2.1 (from package-lock.json). Can you try running npm i @elderjs/elderjs (this will upgrade to latest elderjs version v1.2.4) and then try to build/run it...?

@x4080
Copy link

x4080 commented Dec 28, 2020

@kfrederix Oh I didnt know that, I'll try it thanks for the tip

@x4080
Copy link

x4080 commented Dec 28, 2020

It works nicely,

npm i @elderjs/elderjs

is the key, if I try manually change the package.json to 1.2.4, still wont work

@nickreese
Copy link
Contributor

@x4080 @kfrederix Removed the package lock and updated the package on the template. Thanks. 👍

@x4080
Copy link

x4080 commented Dec 31, 2020

@nickreese Thanks and happy new year !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants