fix: 🐛 use of the package in browsers

[ottonielmatheus]

closes: #93

An export problem that occurs when using the package by bower in the browser makes it useless to use it in browsers.

There is a validation to identify whether the environment is server-side or client-side in the file, but before this file is exported, it tries to use module.exports and require, as it does not have validation in the index.js file, at a time, it breaks and makes it impossible to use the package in the browser.

The fix is simple, but it changes structure, it is necessary that index.js be the old file lib/schema-inspector.js.

[lfreneda]

@Atinux It would be great :)

[ViniciusFXavier]

My fear is that there may be people who are pointing to the file as shown in the DOC, if you make this change of file location it can break for people.

It is very complicated to move the file.

I don't think that would be correct.

An alternative would be to change both package.json and bower.json.
Change "main": "index.js", to "main": "./lib/schema-inspector.js".

Or the option is to change the index.js to better detect whether it is NodeJs or Browser.

But it is necessary to test for both the NodeJs and the Browser.

[ViniciusFXavier]

Or perhaps the best option is to change the index.js to better detect whether it is NodeJs or Browser.

[mattwelke]

I'm going to need someone with Bower expertise to take a look at this issue. I have no experience at all with Bower and not enough time to learn it right now. If someone can take a look at this and post proof that a particular change fixes Bower imports, I can merge it in. @ViniciusFXavier it looks like you're familiar with Bower and you've already decided on the best course of action. Can you provide a simple explanation for me? I'll need something for release notes anyways. I'll release 2.0.1 with the fix today if we can get it all sorted out.

Also, I'm curious... do we know if this is an issue that just came up with 2.0.0 or has this always been an issue? I wasn't around when the library was created many years ago, but since I started having eyes on it in late 2018, I never noticed anyone mention issues using it with Bower.

[mattwelke]

Also note that @Atinux is no longer a maintainer of this library.

[ViniciusFXavier]

Best alternative: #94 (comment)

Add this to index.js must fix the module import.

(function () {
    var root = this;
    var SchemaInspector = new Object();

    if (typeof module !== 'undefined' && module.exports) {
        SchemaInspector = require('./lib/schema-inspector');
        module.exports = SchemaInspector;
    }
    root.SchemaInspector = SchemaInspector;
})();

When doing this will not broken.

<script type="text/javascript" src="bower_components/schema-inspector/lib/schema-inspector.js"></script>

[mattwelke]

@ViniciusFXavier
So it looks like we're got two changes then. One is in this PR. It moves the contents of the JS file under lib into index.js, and the change you just described appears to change how index.js imports the file from lib. If I understand correctly, we would choose one of these changes, and either change would make it so that the library can be imported by Bower users the exact way the README tells them to, right?

[ViniciusFXavier]

I'm going to need someone with Bower expertise to take a look at this issue. I have no experience at all with Bower and not enough time to learn it right now. If someone can take a look at this and post proof that a particular change fixes Bower imports, I can merge it in. @ViniciusFXavier it looks like you're familiar with Bower and you've already decided on the best course of action. Can you provide a simple explanation for me? I'll need something for release notes anyways. I'll release 2.0.1 with the fix today if we can get it all sorted out.

Also, I'm curious... do we know if this is an issue that just came up with 2.0.0 or has this always been an issue? I wasn't around when the library was created many years ago, but since I started having eyes on it in late 2018, I never noticed anyone mention issues using it with Bower.

Hello @mattwelke,
As pointed out by @ottonielmatheus in issue #93, Bower is trying to import the file index.js and the content in it is the same as the link https://github.com/schema-inspector/schema-inspector/blob/master/index.js.

Basically what Bower is doing is what is written in the file bower.json it is importing the file index.js in the browser. It turns out that it has the function module.exports which belongs to NodeJS only,
it's the same as you doing this, this will show the error:

<!DOCTYPE html>
<html>
  <head>
    <meta charset='utf-8'>
    <script type="text/javascript" src="https://rawgit.com/caolan/async/master/dist/async.js"></script>
    <script type="text/javascript" src="https://rawgit.com/atinux/schema-inspector/master/index.js"></script>
  </head>

  <body>
    <h1>Schema Inspector</h1>
  </body>
</html>

[ViniciusFXavier]

@ViniciusFXavier
Parece que temos duas alterações. Um está neste PR. Ele move o conteúdo do arquivo JS em lib para index.js, e a mudança que você acabou de descrever parece mudar como index.js importa o arquivo de lib. Se bem entendi, escolheríamos uma dessas alterações, e qualquer uma das alterações faria com que a biblioteca pudesse ser importada pelos usuários do Bower da maneira exata que o README diz a eles, certo?

Exactly my fear with the content of this PR to change things to index.js is to break other applications that already use this library and it is not with the locked version.

With the suggestion that I gave you will continue to work both previous versions and new ones.

More realistically, the content in the index.js file would be better just like moment and undescore do, the problem is precisely affecting those who already use it.

[mattwelke]

@ViniciusFXavier Nice, I have a decent understanding of the issue now and what we'll do to fix it. @ottonielmatheus thank you sending in this PR but I don't want to fix it by removing the lib file. I don't want people who've followed the readme to install it with Bower to have their setups break (if they were working at any point in the past). I'm going to go with @ViniciusFXavier 's suggestion to improve how index.js detects browser vs. Node.js and how it imports the lib file, so that index.js will always work in browsers. I'll make the change in a separate PR, add some tests with Puppeteer in the CI/CD to assert that the files are usable by the browser directly, and release the fix as 2.0.1. I'll get this done within the next few days.

[ottonielmatheus]

@mattwelke Ok, waiting for fix, thank you guys!!

[ViniciusFXavier]

@ViniciusFXavier Nice, I have a decent understanding of the issue now and what we'll do to fix it. @ottonielmatheus thank you sending in this PR but I don't want to fix it by removing the lib file. I don't want people who've followed the readme to install it with Bower to have their setups break (if they were working at any point in the past). I'm going to go with @ViniciusFXavier 's suggestion to improve how index.js detects browser vs. Node.js and how it imports the lib file, so that index.js will always work in browsers. I'll make the change in a separate PR, add some tests with Puppeteer in the CI/CD to assert that the files are usable by the browser directly, and release the fix as 2.0.1. I'll get this done within the next few days.

@mattwelke I was doing some tests here and I think the best alternative is would be to change both package.json and bower.json.
Change "main": "index.js", to "main": "./lib/schema-inspector.js".

Because in the lines indicated below they already do the same treatment that I gave as an example for the file index.js:

schema-inspector/lib/schema-inspector.js

Lines 1548 to 1553 in e02ea1f

	if (typeof module !== 'undefined' && module.exports) {
	module.exports = SchemaInspector;
	}
	else {
	window.SchemaInspector = SchemaInspector;
	}

This change would work for both CommonJS, RollupJS and Gulp or any other module bundler for JavaScript, and the previous packages using this will continue to work.

[mattwelke]

It looks like 7 years ago, they actually changed Bower to use index.js in order to fix a Bower install issue back then: 2595511

Do we understand why they would have done that and why we now need to do this in reverse to fix it again?

[ViniciusFXavier]

Exactly I saw it too, I just don't understand why they did it.

This PR change to the index #11

Lib has not worked properly in browsers for 7 years, you can go back to the version and test. 😞

The same error will be displayed on the debugger console.

[mattwelke]

I see. Interesting that no one has mentioned this issue in 7 years. I have to admit, I was surprised to see an issue today claiming that the library doesn't work in browsers. We've used it in browsers at my work in production for 4 years. Our workflow is to bundle it in with Webpack. When used in browsers that way, it works perfectly.

I think in 2021, we should actually consider whether Bower support is something we should continue to support going forward. My understanding is that it's an outdated way to build JS applications for browsers. Is this correct?

The example web page already successfully loads the JS file by linking to it with a <script> so we know it can be used in browsers directly already:

[ViniciusFXavier]

Accidental closing

[ottonielmatheus]

An alternative to work around the problem in bower, is to use bower overrides in bower.json.

This solves the problem with the package's index.js, without the need for changes ;)

[ViniciusFXavier]

The example web page already successfully loads the JS file by linking to it with a <script> so we know it can be used in browsers directly already:

This is an alternative.

The Webpack is a smart bundler that can solve the modules, Gulp with Bower is not that smart.

Using Bower and Gulp this happens

😢

[mattwelke]

Yeah if I recall, Webpack can follow the require statements to find the code it needs to bundle in. I guess the code here was laid out in a flawed way that only flexible module bundlers can work with.

I don't like the idea of making changes I perceive as dangerous (like editing main in package.json) because it's very important we don't break anyone's bundling they've already set up. For example, we need to make sure Webpack builds don't break. Many people use Webpack.

@ottonielmatheus I plan to drop support for Bower in the 3.0.0 release that I'll be publishing later this year. Would the workaround you've found satisfy you for now instead of changing the way code is laid out in the repo to allow Bower builds according to the current Bower instructions in the README? I can add the workaround instructions to the README or merge in this PR if it were changed to just be a README update explaining the workaround.

[mattwelke]

@ViniciusFXavier

Maybe just ending support for Bower and adding the suggestion of @ottonielmatheus in the documentation is already better.

This is exactly what I'm proposing.

[mattwelke]

As a reminder of where we're at right now, we need a PR that involves no changes to how the code is structured and adds instructions to the readme explaining how Bower users can use the library in its current state.

[mattwelke]

Consensus is to use workaround instead of changing code. Please change PR to only add workaround instructions to README.md