Skip to content

Support source maps with no file existing on disk #3

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

Closed
rjgotten opened this issue Aug 27, 2018 · 10 comments
Closed

Support source maps with no file existing on disk #3

rjgotten opened this issue Aug 27, 2018 · 10 comments

Comments

@rjgotten
Copy link

rjgotten commented Aug 27, 2018

Source map destination files do not need to exist on disk. Source text can be embedded within the map file instead.

Current codecs all check for validity of a file on disk.
While it's possible to create 'unchecked' variants of these, later on inside the loader's own processing we arrive at another file check:

// non-existant file implies error
else if (!fs.existsSync(decoded) || !fs.statSync(decoded).isFile()) {
return getNamedError('Cannot find file at absolute path:\n' + decoded);
}

This we cannot resolve from user-land. It basically makes the entire loader useless if you're dealing with libraries that use embedded source maps.

@rjgotten rjgotten changed the title Support source maps with no existant file on disk Support source maps with no file existing on disk Aug 27, 2018
@bholloway
Copy link
Owner

Okay @rjgotten I think we can accomodate. You have some well structured points so I will just reply.

"possible to create 'unchecked' variants of these [codecs]"

I think you are on the right track. Trying to infer how the source-map sources are constructed really requires project knowledge.

Originally I went for the general solution but it is only reliable if we check the files exist.

"Current codecs all check for validity of a file on disk."

Where a file check is necessary I think it makes sense to restrict it to the codec. I would need to survey myself.

🤔 This would probably be the best "separation of concerns" but maybe a risky change without tests. I would need to look properly.

Failing that I could add a flag that allows you to opt-out of the tests. There is precedent with the abstract flag. So maybe a force flag?

Let me know what your preference is.

@rjgotten
Copy link
Author

Maybe something slightly more intelligent: skip only the verification of whether files exist on disk, provided the source map already has the file content embedded as a sourcesContent array.

Ofcourse, a force flag is easier to implement. But imho not quite as nice.
And potentially more dangerous to end up with an invalid map.

@bholloway
Copy link
Owner

@rjgotten your suggestion is interesting but I suspect unsuitable - It is not always possible to opt-out of sourcesContent. In my own use case sourcesContent exists but the actual sources also need to exist so I can delete the sourcesContent downstream.

Fundamentally I think that most of the codecs only work if the files are present. Otherwise there is no basis to differentiate them.

I think we can remove the additional check you originally highlighted. I am thinking it was probably originally done to validate the output of custom codecs. But as you have found it is over-restrictive. I am inclined to just remove it as a breaking change and publish a @next dist-tag for you to play with.

@bholloway
Copy link
Owner

@rjgotten looking at the code just now I recall the concept of an abstract codec.

This is usually used for sources such as the webpack bootstrap that can be recognised but will not be amended in any way.

If you don't intend to amend the sources in question then you could just prepend a codec that is abstract: true and simple returns decode(): true for just those paths. This will ensure they are retained as is.

However if you intend to adjust these sources then we will need to make a change as discussed.

@bholloway
Copy link
Owner

@rjgotten please try adjust-sourcemap-loader@next and use the custom resolvers as you first implied.

@bholloway
Copy link
Owner

@rjgotten should we release this? how did it go?

@rjgotten
Copy link
Author

rjgotten commented Sep 7, 2018

I have not yet been able to revisit the part of the project in question, where I needed to apply this.
Sorry. :(

@bholloway
Copy link
Owner

No rush @rjgotten. Let me know when you do.

@bholloway
Copy link
Owner

I'm going to release this on risk @rjgotten 🤞

@bholloway
Copy link
Owner

Should be fixed by #6 🤞

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

No branches or pull requests

2 participants