Skip to content

fix(commonjs): fixed access to dynamic node_modules module with subfolder (i.e 'logform/json')#601

Merged
shellscape merged 1 commit intorollup:masterfrom
danielgindi:bugfix/misinterpreted_slash
Oct 11, 2020
Merged

fix(commonjs): fixed access to dynamic node_modules module with subfolder (i.e 'logform/json')#601
shellscape merged 1 commit intorollup:masterfrom
danielgindi:bugfix/misinterpreted_slash

Conversation

@danielgindi
Copy link
Contributor

Rollup Plugin Name: commonjs

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

Description

In cases where there's access to a dynamically loaded module, with a slash in the middle (i.e. require('logform/json')), the result was empty.
This is due to a regex that matched cases it shouldn't have.

@danielgindi danielgindi changed the title fix(commonjs): fixed access to with subfolder (i.e 'logform/json') fix(commonjs): fixed access to dynamic module with subfolder (i.e 'logform/json') Oct 9, 2020
@danielgindi danielgindi force-pushed the bugfix/misinterpreted_slash branch 2 times, most recently from fe6e9e9 to 4a5cf9b Compare October 9, 2020 07:33
@danielgindi
Copy link
Contributor Author

I've improved that regex even more, to be somewhat readable

@danielgindi
Copy link
Contributor Author

@shellscape I don't mean to be pushy but... ;-)

@danielgindi danielgindi force-pushed the bugfix/misinterpreted_slash branch from 4a5cf9b to 8937563 Compare October 11, 2020 07:38
@danielgindi danielgindi changed the title fix(commonjs): fixed access to dynamic module with subfolder (i.e 'logform/json') fix(commonjs): fixed access to dynamic node_modules module with subfolder (i.e 'logform/json') Oct 11, 2020
@shellscape
Copy link
Collaborator

@danielgindi Ideally we'd like to get at least one more pair of eyes on this before merge. Could you request a review from a few other folks?

@danielgindi
Copy link
Contributor Author

@danielgindi Ideally we'd like to get at least one more pair of eyes on this before merge. Could you request a review from a few other folks?

Of course :-)

@LarsDenBakker LarsDenBakker self-requested a review October 11, 2020 16:51
Copy link
Contributor

@LarsDenBakker LarsDenBakker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shellscape shellscape merged commit ec70687 into rollup:master Oct 11, 2020
@danielgindi danielgindi deleted the bugfix/misinterpreted_slash branch October 11, 2020 17:05
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.

3 participants