Skip to content

Allow threads.js to function even with webpack node externals - #224

Merged
andywer merged 2 commits into
andywer:masterfrom
chingyawhao:master
Mar 31, 2020
Merged

andywer merged 2 commits into
andywer:masterfrom
chingyawhao:master

Conversation

@chingyawhao

@chingyawhao chingyawhao commented Mar 24, 2020 •

Copy link
Copy Markdown
Contributor

Solving #222

@chingyawhao

Copy link
Copy Markdown
Contributor Author

@andywer this would handle the absolute path returned by rebaseScriptPath() that you mentioned right?
I am assuming the absolute path you are talking about is something like /Users/chingyawhao/Documents

@andywer

andywer commented Mar 25, 2020

Copy link
Copy Markdown
Owner

Cool! Pushed a commit introducing some minor code improvements. Yes, that was the absolute path that I meant – only replaced the check with path.isAbsolute(), so we can be sure it works with Windows paths, too.

That's some great stuff right here 🙂

@chingyawhao

Copy link
Copy Markdown
Contributor Author

Thanks @andywer

I have no idea how to do testing for this scenario though, I tried adding webpack-node-externals to the existing node test before these changes but it did not fail

@chingyawhao chingyawhao reopened this Mar 27, 2020
@andywer

andywer commented Mar 31, 2020

Copy link
Copy Markdown
Owner

@chingyawhao Yeah, I was thinking the same. Unfortunately I don't really have time to look into this right now either…

We should probably just merge the fix. Do you need it urgently, btw?

@chingyawhao

Copy link
Copy Markdown
Contributor Author

I'm able to make the changes inside node_modules so its no rush

@andywer
andywer merged commit cb3cbf9 into andywer:master Mar 31, 2020
jmaleonard referenced this pull request in jmaleonard/threadsx Jul 11, 2026
Co-authored-by: Ching Yaw Hao <ching.yh@mydidian.com>
Co-authored-by: Andy Wermke <github@andywer.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants