[TIMOB-27206] Unable to require files under node_modules #131

brentonhouse · 2020-01-07T18:41:05Z

Added support for finding files located in node_modules
Some formatting changes occurred due to eslint scripts (eslint config was not changed)

Note:

There are several areas that I need to investigate to improve performance of this. Right now, it is checking to see if a file exists but is having to do it multiple times for each request because it doesn't know what files exists. I need to get access to (or create) a cache of JS files and native modules so that LiveView can just check this instead of the more time consuming check I implemented.

brentonhouse · 2020-02-25T13:45:49Z

@ewanharris - Is this PR waiting on anything? LiveView doesn't work without it if you use npm modules at all.

ewanharris · 2020-02-25T13:49:55Z

@brentonhouse, I just haven't had time to review it yet. I gave preference to Ebenezer's PRs as he will be finishing on our team at the start of next week and I wanted to get him some product development before switching.

What ticket is this PR associated with?

brentonhouse · 2020-02-25T13:53:55Z

associated with: https://jira.appcelerator.org/browse/TIMOB-27206

ewanharris

Took an initial run through the code changes, need to pull this down and test it

ewanharris · 2020-03-06T08:38:30Z

lib/platform/require.js

-		file = Ti.Filesystem.getFile(path);
+	const idPath = path.parse(id);
+
+	// TIBUG: Fix for path.parse bug


Is there a ticket for this bug? I'd prefer to schedule a fix alongside this change to liveview rather than add fixes in liveview

Not sure if there is a ticket for this. It probably involves replacing our custom "path" module with the open-source path module.

Filed TIMOB-27899 to fix this in 9.1.0

lib/fserver.js

ewanharris · 2020-03-06T08:49:48Z

lib/fserver.js

 		// want to add these into a classic apps Resources dir
-		const buildNodeModulesPath = join(RESOURCE_DIR, '..', 'build', 'node_modules', uri);
+		const buildNodeModulesPath = join(RESOURCE_DIR, '..', 'build', 'node_modules', uri + urlPath.ext);


I need to double check but I think we can remove this now as we're bundling the sdk core JS code

ok. I can remove it.

ewanharris · 2020-03-06T08:50:30Z

lib/fserver.js

 		} else {

-			let filename = join(RESOURCE_DIR, uri);
+			let filename = join(RESOURCE_DIR, uri + urlPath.ext);
+			log('[LiveView]'.green, 'filename: ' + filename);


I assume these will be removed before merge?

Co-Authored-By: Ewan Harris <[email protected]>

ewanharris

Testing locally it seems like KS-v2 is erroring out on startup on Android with these changes, need to dig in to find the problem.

@brentonhouse, is your sample app working fine on Android?

ewanharris · 2020-05-18T13:29:11Z

lib/fserver.js

+		const platformFilepath = join(RESOURCE_DIR, requestPlatform, uri + urlPath.ext);
+		const platformFilepath2 = join(RESOURCE_DIR, requestPlatform, uri, 'index' + urlPath.ext);
+		const overridePath = join(__dirname, 'overrides', uri + urlPath.ext);
+		const projectNodeModulesMainPath = join(RESOURCE_DIR, requestPlatform, 'node_modules', uri + urlPath.ext);


Looking up under Resources/<platform> is only correct for Alloy apps, we need to ensure this works for both Classic and Alloy.

Also, after testing this out I think only going on node_modules level deep might also be problematic, seeing as the algorithm we use in the SDK is pretty much the same as node (minus the loading of .node files) I think we could maybe use require.resolve with the paths set to the directory name

lib/fserver.js

Co-authored-by: Ewan Harris <[email protected]>

…w into node_modules_update

ewanharris · 2020-09-30T14:19:44Z

@brentonhouse I've been trying this out in classic and there seems to be some issues around requiring files. If I create Resources/test.js and then do require('test') an error gets thrown, but require('test.js') works ok.

However if I then rebuild require('test') will work just fine, I'm guessing this is down to the on-device lookup that's performed in the liveview runtime and there being a difference in what gets sent to the liveview server.

I haven't tested yet but I'm going to guess that'll extend to alloy libs too

brentonhouse · 2020-09-30T14:41:59Z

@ewanharris - I have had to change this around significantly in order for it to work with all the latest code. I ended up just doing the changes in the combined file instead of the split code files so I would work faster and test things easier. I would have to back track this to each of the separate files unless we are finally ready to get rid of those.

ewanharris · 2020-10-01T16:56:56Z

@brentonhouse, I think I'm fine with removing the multiple files + concat build process. However I don't think it should be done in this PR as it would just add a ton of noise and make it difficult to review the require resolution changes

ewanharris · 2020-11-10T09:56:49Z

@brentonhouse, if you want. Send me the build/liveview.js you have that's working and I can work on applying the changes back to the right places

brentonhouse added 2 commits January 6, 2020 16:16

Merge remote-tracking branch 'upstream/master'

b8ba9c9

updates to support node_modules

cf044d1

brentonhouse requested review from sgtcoolguy and ewanharris January 7, 2020 18:41

brentonhouse self-assigned this Jan 7, 2020

brentonhouse added 2 commits January 7, 2020 17:04

cleaning up code

20b56e3

generated babel file

e144cf0

brentonhouse added the bug label Feb 25, 2020

brentonhouse changed the title ~~Node modules update~~ [TIMOB-27206] Unable to require files under node_modules Feb 25, 2020

Merge branch 'master' into node_modules_update

eb40403

ewanharris reviewed Mar 30, 2020

View reviewed changes

brentonhouse and others added 2 commits April 29, 2020 10:47

Update lib/fserver.js

f950547

Co-Authored-By: Ewan Harris <[email protected]>

requested changes for PR

0384984

ewanharris reviewed May 18, 2020

View reviewed changes

lib/fserver.js Outdated Show resolved Hide resolved

brentonhouse and others added 5 commits May 29, 2020 06:52

formatting fix

3b00f54

Update lib/fserver.js

045a427

Co-authored-by: Ewan Harris <[email protected]>

Merge branch 'node_modules_update' of github.com:brentonhouse/livevie…

a17bea4

…w into node_modules_update

fixed issue with parent null

63b6d53

updated built liveview file

5fac3db

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TIMOB-27206] Unable to require files under node_modules #131

[TIMOB-27206] Unable to require files under node_modules #131

brentonhouse commented Jan 7, 2020 •

edited

Loading

brentonhouse commented Feb 25, 2020

ewanharris commented Feb 25, 2020

brentonhouse commented Feb 25, 2020

ewanharris left a comment

ewanharris Mar 6, 2020

brentonhouse Apr 29, 2020

ewanharris May 18, 2020

ewanharris Mar 6, 2020

brentonhouse Apr 29, 2020

ewanharris Mar 6, 2020

brentonhouse May 29, 2020

ewanharris left a comment

ewanharris May 18, 2020

ewanharris commented Sep 30, 2020

brentonhouse commented Sep 30, 2020

ewanharris commented Oct 1, 2020

ewanharris commented Nov 10, 2020

[TIMOB-27206] Unable to require files under node_modules #131

Are you sure you want to change the base?

[TIMOB-27206] Unable to require files under node_modules #131

Conversation

brentonhouse commented Jan 7, 2020 • edited Loading

brentonhouse commented Feb 25, 2020

ewanharris commented Feb 25, 2020

brentonhouse commented Feb 25, 2020

ewanharris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewanharris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewanharris commented Sep 30, 2020

brentonhouse commented Sep 30, 2020

ewanharris commented Oct 1, 2020

ewanharris commented Nov 10, 2020

brentonhouse commented Jan 7, 2020 •

edited

Loading