Add failing test for top level transforms and symlinked node modules #1365

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@ivantsepp

Adds a failing test with using a transform on top-level files on symlinked node modules.

The test failure is coming from passing in the realpath and module-deps uses this path to determine if the file is top level.

A possible solution is to not pass in the realpath and calculate it instead in module-deps. I think this makes sense as module-deps would want to know the realpath and the resolved path of the file.

@ivantsepp

This comment has been minimized.

Show comment
Hide comment
@ivantsepp

ivantsepp Sep 23, 2015

Any updates on this? Is the test in this PR expected behavior?

Any updates on this? Is the test in this PR expected behavior?

@ivantsepp ivantsepp changed the title from Add test for top level transforms and symlinked node modules to Add failing test for top level transforms and symlinked node modules Sep 23, 2015

@jmm

This comment has been minimized.

Show comment
Hide comment
@jmm

jmm Sep 23, 2015

Collaborator

@ivantsepp Thanks for this! I haven't tried your test (and unfortunately I can't manage to fully "get it" just from the description), but there's some discussion going on in #1386 that you may be interested in.

Collaborator

jmm commented Sep 23, 2015

@ivantsepp Thanks for this! I haven't tried your test (and unfortunately I can't manage to fully "get it" just from the description), but there's some discussion going on in #1386 that you may be interested in.

@zertosh

This comment has been minimized.

Show comment
Hide comment
@zertosh

zertosh Oct 26, 2015

Member

I guess this is fixed by #1392

Member

zertosh commented Oct 26, 2015

I guess this is fixed by #1392

@zertosh zertosh closed this Oct 26, 2015

@chrisirhc

This comment has been minimized.

Show comment
Hide comment
@chrisirhc

chrisirhc Oct 26, 2015

Contributor

@zertosh Yep, though #1392 hasn't been merged.

Contributor

chrisirhc commented Oct 26, 2015

@zertosh Yep, though #1392 hasn't been merged.

@zertosh

This comment has been minimized.

Show comment
Hide comment
@zertosh

zertosh Oct 26, 2015

Member

@chrisirhc cool! Lets let ppl chime in on #1413 since that'll bump browserify to v12, so then #1392 can ride along with that.

Member

zertosh commented Oct 26, 2015

@chrisirhc cool! Lets let ppl chime in on #1413 since that'll bump browserify to v12, so then #1392 can ride along with that.

@joews joews referenced this pull request in jmreidy/grunt-browserify Jan 26, 2016

Closed

Release with browserify ^12+ #375

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment