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

[TIMOB-24148] iOS: Add note for CommonJS modules colliding with Ti namespaces #8608

Closed
wants to merge 4 commits into from

Conversation

hansemannn
Copy link
Collaborator

@cb1kenobi
Copy link
Contributor

+1

@sgtcoolguy
Copy link
Contributor

sgtcoolguy commented Nov 16, 2016

I'd tweak this a little bit. The wording seems to indicate that there was a failure and to always use the './' prefix.

What we want the user to know is that there is a clash between a native module with the given id and a JS file with the module id as it's base name - and that we're giving preference to the native module. If they intended to address the JS file, they should use the prefix to make it explicit.

I'd also worry that even if they fix their usage but have a JS file whose base name matches a native module id they'll keep seeing this message - and we don't spell out for them that to avoid the message they need to effectively rename the js file (Which may be another reason why we'd want this at a WARNING level instead of ERROR)

@hansemannn
Copy link
Collaborator Author

What we want the user to know is that there is a clash between a native module with the given id and a JS file with the module id as it's base name - and that we're giving preference to the native module. If they intended to address the JS file, they should use the prefix to make it explicit.

I have to confess that it's hard for me to find the correct wording on this. As the use-case only effects a very specific kind of local CommonJS modules with no sub-directories and clashing names with core-modules, the suggested ./ prefix would always make sense for that kind of file.

I'd also worry that even if they fix their usage but have a JS file whose base name matches a native module id they'll keep seeing this message - and we don't spell out for them that to avoid the message they need to effectively rename the js file

Native modules won't have a JS file in the app like CommonJS modules have, so the won't collide. I can setup a separate test-case for that if you want

(Which may be another reason why we'd want this at a WARNING level instead of ERROR)

Agree, I'll change it to [WARN]

@sgtcoolguy
Copy link
Contributor

Merged manually by copying the changes and tweaking wording some (and bumping down to WARN).

@sgtcoolguy sgtcoolguy closed this Nov 29, 2016
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.

None yet

3 participants